Conversation
…ls as empty, check undertitel field Co-authored-by: pethers <[email protected]>
Co-authored-by: pethers <[email protected]>
…c test name Co-authored-by: pethers <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR addresses three quality issues in motion article generation:
- Issue #454: Fixes "Unknown (Unknown)" author/party by treating 'Unknown' as a sentinel and falling back to text parsing from
undertitel,summary,notis, andfullTextfields - Issue #452: Adds MCP fail-fast mode via
--require-mcpflag (default: enabled) to prevent silent degraded generation when the MCP server is unavailable - Issue #462: Eliminates repetitive proposition titles by grouping motions under their parent proposition reference, rendering a single
<h3>group heading with<h4>sub-entries for individual motions
The changes improve content quality by ensuring accurate author attribution, reliable data fetching, and better visual organization of grouped motions.
Changes:
- Added
undertitelfield toRawDocumentinterface and test mocks as a primary source for author/party extraction - Implemented MCP fail-fast logic with
--require-mcpflag that exits with code 1 when MCP warm-up fails (unless explicitly disabled) - Introduced motion grouping functions (
extractParentPropRef,groupMotionsByProposition,renderMotionEntry) to deduplicate repetitive proposition-response titles - Added
responsesToPropandindependentMotionslabels to all 14 language translations - Updated author/party extraction logic to treat 'Unknown' sentinel as missing data, triggering text parsing fallbacks
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/types/content.ts | Added responsesToProp function and independentMotions string to ContentLabelSet interface for motion grouping |
| scripts/generate-news-enhanced.ts | Added --require-mcp flag parsing, fail-fast logic in getSharedClient(), and exported requireMcpArg for testing |
| scripts/data-transformers.ts | Added undertitel field to RawDocument; updated generateEnhancedSummary, renderMotionEntry, generateMotionsContent, and generateDocumentIntelligenceAnalysis to handle 'Unknown' sentinel; implemented motion grouping by proposition with extractParentPropRef, groupMotionsByProposition, and renderMotionEntry functions; added i18n labels for all 14 languages |
| tests/data-transformers.test.ts | Added undertitel field to mock interfaces; added 3 tests for author/party extraction from notis/undertitel/summary; added 4 tests for motion grouping scenarios (grouped, single group with multiple motions, mixed grouped/independent, flat fallback); verified new labels exist in all languages |
| tests/generate-news-enhanced-part1.test.ts | Added test verifying requireMcpArg exports and defaults to true; added requireMcpArg to GenerateNewsEnhancedModule interface |
scripts/data-transformers.ts
Outdated
| if (!authorName || authorName === 'Unknown') { | ||
| authorName = ''; | ||
| const rawText = motion.undertitel || motion.summary || motion.notis || motion.fullText || ''; | ||
| const parsed = parseMotionAuthorParty(rawText); | ||
| if (parsed) { authorName = parsed.author; partyName = parsed.party; } | ||
| } | ||
| if (partyName === 'Unknown') partyName = ''; |
There was a problem hiding this comment.
When parsing author/party from text fallback, the code unconditionally overwrites partyName even if it was already set to a valid value (not 'Unknown'). This could lose valid party data from motion.parti when only the author name is missing.
Consider preserving the existing party if it's valid:
- Only update
partyNamefrom parsed result if it was empty or 'Unknown' - Or check if
partyNameis already valid before the parse attempt
| if (!authorName || authorName === 'Unknown') { | |
| authorName = ''; | |
| const rawText = motion.undertitel || motion.summary || motion.notis || motion.fullText || ''; | |
| const parsed = parseMotionAuthorParty(rawText); | |
| if (parsed) { authorName = parsed.author; partyName = parsed.party; } | |
| } | |
| if (partyName === 'Unknown') partyName = ''; | |
| if (partyName === 'Unknown') partyName = ''; | |
| if (!authorName || authorName === 'Unknown') { | |
| authorName = ''; | |
| const rawText = motion.undertitel || motion.summary || motion.notis || motion.fullText || ''; | |
| const parsed = parseMotionAuthorParty(rawText); | |
| if (parsed) { | |
| authorName = parsed.author; | |
| if (!partyName) partyName = parsed.party; | |
| } | |
| } |
scripts/data-transformers.ts
Outdated
| if (!propRef) continue; // handle independent block after the loop | ||
| const firstTitle = groupMotions[0]?.titel || groupMotions[0]?.title || ''; | ||
| // Strip "med anledning av prop. YYYY/YY:NNN " prefix to reveal proposition title | ||
| const propTitle = firstTitle.replace(/^med anledning av prop(?:ositionen?)?\.\s+\S+\s*/i, '').trim(); |
There was a problem hiding this comment.
The regex for stripping the proposition prefix requires at least one whitespace after the period (\s+), which might fail if titles lack spacing after the period. Consider making the whitespace optional (\s*) to handle edge cases like "med anledning av prop.2025/26:118" without a space.
However, verify whether the actual data from the Riksdag API consistently includes spaces. If it does, this is a minor issue.
scripts/data-transformers.ts
Outdated
| const independentLabel = L(lang, 'independentMotions'); | ||
| content += `\n <div class="motion-group">\n`; | ||
| content += ` <h3>${escapeHtml(String(independentLabel))}</h3>\n`; | ||
| independent.forEach(motion => { content += renderMotionEntry(motion, lang, true); }); |
There was a problem hiding this comment.
Independent motions use grouped=true which displays dok_id (e.g., "HD023900") as the <h4> heading instead of the motion's title (e.g., "Klimatpolitik och havsnivåer"). While this maintains consistent HTML structure, it may reduce readability for end users since independent motions don't have repetitive titles that need deduplication.
Consider modifying renderMotionEntry to accept a third parameter indicating whether to use dok_id or title for the heading when in grouped mode, allowing independent motions to show their full titles for better user experience.
| independent.forEach(motion => { content += renderMotionEntry(motion, lang, true); }); | |
| // Independent motions don't need title deduplication; render with full titles instead of dok_id | |
| independent.forEach(motion => { content += renderMotionEntry(motion, lang, false); }); |
|
@copilot apply changes based on the comments in this thread and merge with main. another PR been merged so all conflicts need to be fixed. This branch has conflicts that must be resolved scripts/generate-news-enhanced.ts |
… pattern Co-authored-by: pethers <[email protected]>
… throw pattern Co-authored-by: pethers <[email protected]>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
scripts/data-transformers.ts
Outdated
| const parsed = parseMotionAuthorParty(rawText); | ||
| if (parsed) { authorName = parsed.author; partyName = partyName || parsed.party; } | ||
| if (parsed) { | ||
| if (parsed.author && (!authorName || authorName === 'Unknown')) authorName = parsed.author; |
There was a problem hiding this comment.
The check for authorName === 'Unknown' at line 2262 is unreachable because line 2254 already transforms 'Unknown' to null. When motion.intressent_namn === 'Unknown', line 2254 evaluates to (false ? motion.intressent_namn : null) || motion.author || '', which can never result in the string 'Unknown'. The condition should be simplified to just !authorName since 'Unknown' has already been filtered out by the ternary at line 2254.
There was a problem hiding this comment.
Fixed in b888c85. Both || authorName === 'Unknown' guards removed — line 2254 already converts 'Unknown' to null, so the condition on line 2258 simplifies to !authorName || !partyName and the patching guard on line 2262 simplifies to if (parsed.author && !authorName).
scripts/data-transformers.ts
Outdated
| const rawText = motion.summary || motion.notis || motion.fullText || motion.titel || motion.rubrik || ''; | ||
| // Fire fallback when EITHER author or party is missing — covers the party-only sentinel case | ||
| // where intressent_namn is valid but parti was 'Unknown' and stripped to ''. | ||
| if (!authorName || authorName === 'Unknown' || !partyName) { |
There was a problem hiding this comment.
The check for authorName === 'Unknown' at line 2258 is redundant because line 2254 already transforms any 'Unknown' value to null. The condition !authorName || authorName === 'Unknown' can be simplified to just !authorName since the transformation at line 2254 ensures authorName cannot be the string 'Unknown' at this point.
|
@copilot apply changes based on the comments in this thread |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
…ds (already null-converted at line 2254) Co-authored-by: pethers <[email protected]>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
…tinel missed Co-authored-by: pethers <[email protected]>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
scripts/data-transformers.ts
Outdated
| let authorName = (motion.intressent_namn !== 'Unknown' ? motion.intressent_namn : null) || motion.author || ''; | ||
| let partyName = (motion.parti !== 'Unknown' ? motion.parti : '') || ''; | ||
| if (!authorName || authorName === 'Unknown') { | ||
| const rawText = motion.summary || motion.notis || motion.fullText || motion.titel || motion.rubrik || ''; | ||
| // Fire fallback when EITHER author or party is missing — covers the party-only sentinel case | ||
| // where intressent_namn is valid but parti was 'Unknown' and stripped to ''. | ||
| if (!authorName || !partyName) { | ||
| const rawText = motion.undertitel || motion.summary || motion.notis || motion.fullText || motion.titel || motion.rubrik || ''; | ||
| const parsed = parseMotionAuthorParty(rawText); | ||
| if (parsed) { authorName = parsed.author; partyName = partyName || parsed.party; } | ||
| if (parsed) { | ||
| if (parsed.author && !authorName) authorName = parsed.author; | ||
| if (parsed.party && !partyName) partyName = parsed.party; | ||
| } |
There was a problem hiding this comment.
authorName can still become the truthy sentinel 'Unknown' via motion.author (enrichment defaults author to 'Unknown'). In that case if (!authorName || !partyName) won’t treat the author as missing, and the selective patching block won’t overwrite it (!authorName is false), so the UI can still render an unlocalized Unknown even when parseMotionAuthorParty() could extract a real author. Treat 'Unknown' from BOTH intressent_namn and author as missing (e.g., normalize both to ''/null or use an isSentinel() check in the fallback trigger and patch conditions), and ensure the final fallback localizes when the value is 'Unknown'.
There was a problem hiding this comment.
Fixed in ff3134b. Line 2254 now also strips 'Unknown' from motion.author: (motion.author !== 'Unknown' ? motion.author : null) || '' — so both enrichment fields are treated as missing when they carry the sentinel, and the !authorName fallback trigger fires correctly in all cases.
| it('extracts party from notis when author is valid but parti is Unknown sentinel', () => { | ||
| // Simulates enrichDocumentsWithContent setting intressent_namn correctly but parti='Unknown' | ||
| const content = generateArticleContent({ | ||
| motions: [{ | ||
| titel: 'Test motion', | ||
| intressent_namn: 'Ulrika Liljeberg', | ||
| parti: 'Unknown', | ||
| notis: 'Motion till riksdagen 2025/26:456 av Ulrika Liljeberg (C) om något viktigt.', | ||
| url: '#', | ||
| dok_id: 'MOT_PARTY_ONLY', | ||
| }] | ||
| } as MockArticlePayload, 'motions', 'en') as string; | ||
| expect(content).toContain('Ulrika Liljeberg'); | ||
| expect(content).toContain('(C)'); | ||
| expect(content).not.toContain('Unknown (Unknown)'); | ||
| }); |
There was a problem hiding this comment.
The new sentinel regression test doesn’t cover the real enriched-data shape where enrichDocumentsWithContent also sets motion.author (often to the truthy 'Unknown' sentinel). Because that value can block fallback parsing/patching, add a test case where intressent_namn: 'Unknown' and author: 'Unknown' (plus parti: 'Unknown') and verify author+party are extracted from titel/notis and Unknown (Unknown) never appears.
There was a problem hiding this comment.
Added in ff3134b: 'extracts author and party from notis when both intressent_naam and author are Unknown sentinels' — uses intressent_naam: 'Unknown', author: 'Unknown', parti: 'Unknown', notis containing 'Erik Andersson (KD)' — asserts both fields extracted and Unknown (Unknown) never appears. Also added author? to MockArticlePayload.motions.
|
@copilot apply changes based on the comments in this thread |
…sion test Co-authored-by: pethers <[email protected]>
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
enrichDocumentsWithContentwrites'Unknown'as a truthy sentinel forintressent_namn,author, andparti. TheparseMotionAuthorParty()fallback only fired whenauthorNamewas empty, so motions with a sentinel party (but valid author) silently rendered "(Unknown)", and motions wheremotion.authorwas the'Unknown'sentinel bypassed the fallback entirely because the value was truthy.Changes
scripts/data-transformers.ts—renderMotionEntry'Unknown'sentinel tonull/''forintressent_namn,author(fallback source), andpartibefore any fallback logic — all three enrichment-defaulted sentinels are now normalised!authorName || !partyName(either field missing fires text parsing)parsed.author &&,parsed.party &&) prevent overwriting with empty string from malformed text like"av (M)"undertitel(riksdag subtitle, most reliable) →summary→notis→fullTextmotion.parti === 'Unknown'in Coalition Dynamics byParty stats now buckets as'other'tests/data-transformers.test.tsintressent_namn+parti: 'Unknown'→ party correctly extracted fromnotis;Unknown (Unknown)absentintressent_namn: 'Unknown',author: 'Unknown',parti: 'Unknown'→ both author and party extracted fromnotis;Unknown (Unknown)absent (covers the real enriched-data shape)MockArticlePayload.motionswithnotis?,summary?,undertitel?,author?(fixesTS2352)tests/generate-news-enhanced-mcp-abort.test.tsbeforeEachto vitest import (was causingTS2304)beforeAllnow throws on import failure instead of silently settingmoduleExports = null; tests assertexpect(moduleExports).not.toBeNull()and use!non-null assertionsOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.