Add client-side search, pagination, and URL state to news index pages#701
Add client-side search, pagination, and URL state to news index pages#701
Conversation
- Add search, searchPlaceholder, loadMore, showing to I18nStrings interface - Add translations for all 14 languages in constants.ts - Update template.ts: search input, 20-article pagination with Load More, debounced search, URL state management (?q=, ?type=, ?topic=, ?sort=), WCAG 2.1 AA ARIA labels and focus management - Add CSS for search input, pagination controls, load-more button Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
github-actions
bot
added
html-css
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
There was a problem hiding this comment.
Pull request overview
This PR updates the generated multi-language news index pages to remain usable with 900+ articles by adding client-side search, progressive “load more” pagination, and URL query-state syncing.
Changes:
- Add debounced title search input and integrate it into existing type/topic/sort filtering.
- Add progressive pagination (PAGE_SIZE=20) with a “Load more” button, plus an aria-live “showing X of Y” counter.
- Add translations for the new UI strings across all 14 languages and add scoped CSS for the new controls.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| styles.css | Adds .news-page-scoped styles for search input, counter, and load-more button. |
| scripts/generate-news-indexes/types.ts | Extends I18nStrings with new keys for search/pagination UI. |
| scripts/generate-news-indexes/template.ts | Implements search, pagination rendering, focus management, and URL param syncing in generated index HTML/JS. |
| scripts/generate-news-indexes/constants.ts | Adds translations for the new i18n keys across all supported languages. |
| if (btn) { | ||
| if (total > visibleCount) { | ||
| btn.style.display = 'inline-block'; | ||
| btn.setAttribute('aria-label', i18nLoadMore); |
There was a problem hiding this comment.
i18nLoadMore is built using escapeHtml(...) and then used via btn.setAttribute('aria-label', i18nLoadMore). Because HTML entities are not decoded when setting attributes from JS, languages containing apostrophes (e.g. French) will end up with a literal "'" in the aria-label. Define i18nLoadMore as a proper JS string literal (e.g. via JSON.stringify(...)) or avoid resetting the aria-label since it’s already set in the HTML.
| btn.setAttribute('aria-label', i18nLoadMore); |
| if (params.has('type')) document.getElementById('filter-type').value = params.get('type'); | ||
| if (params.has('topic')) document.getElementById('filter-topic').value = params.get('topic'); | ||
| if (params.has('sort')) document.getElementById('filter-sort').value = params.get('sort'); | ||
| if (params.has('q')) document.getElementById('search-input').value = params.get('q'); |
There was a problem hiding this comment.
readURLParams() assigns query param values directly into the <select>/<input> values without validating them against allowed options. Invalid values (e.g. ?type=bogus or ?type=) can force the UI into a zero-results state and desync the selects from their option lists. Consider whitelisting allowed values (or checking for a matching <option value="...">) and falling back to defaults when invalid.
| if (params.has('type')) document.getElementById('filter-type').value = params.get('type'); | |
| if (params.has('topic')) document.getElementById('filter-topic').value = params.get('topic'); | |
| if (params.has('sort')) document.getElementById('filter-sort').value = params.get('sort'); | |
| if (params.has('q')) document.getElementById('search-input').value = params.get('q'); | |
| // Safely apply type filter from URL if it matches an existing option | |
| var typeSelect = document.getElementById('filter-type'); | |
| var typeParam = params.get('type'); | |
| if (typeSelect && typeSelect.tagName === 'SELECT' && typeParam) { | |
| var typeOptions = Array.prototype.slice.call(typeSelect.options); | |
| var hasTypeOption = typeOptions.some(function (option) { | |
| return option.value === typeParam; | |
| }); | |
| if (hasTypeOption) { | |
| typeSelect.value = typeParam; | |
| } | |
| } | |
| // Safely apply topic filter from URL if it matches an existing option | |
| var topicSelect = document.getElementById('filter-topic'); | |
| var topicParam = params.get('topic'); | |
| if (topicSelect && topicSelect.tagName === 'SELECT' && topicParam) { | |
| var topicOptions = Array.prototype.slice.call(topicSelect.options); | |
| var hasTopicOption = topicOptions.some(function (option) { | |
| return option.value === topicParam; | |
| }); | |
| if (hasTopicOption) { | |
| topicSelect.value = topicParam; | |
| } | |
| } | |
| // Safely apply sort filter from URL if it matches an existing option | |
| var sortSelect = document.getElementById('filter-sort'); | |
| var sortParam = params.get('sort'); | |
| if (sortSelect && sortSelect.tagName === 'SELECT' && sortParam) { | |
| var sortOptions = Array.prototype.slice.call(sortSelect.options); | |
| var hasSortOption = sortOptions.some(function (option) { | |
| return option.value === sortParam; | |
| }); | |
| if (hasSortOption) { | |
| sortSelect.value = sortParam; | |
| } | |
| } | |
| // Apply search query directly; arbitrary text is expected here | |
| var searchInput = document.getElementById('search-input'); | |
| var qParam = params.get('q'); | |
| if (searchInput && qParam !== null && 'value' in searchInput) { | |
| searchInput.value = qParam; | |
| } |
| filteredArticles = filtered; | ||
| renderArticles(filteredArticles); | ||
| visibleCount = PAGE_SIZE; | ||
| updateURL(); | ||
| renderPage(); | ||
| } |
There was a problem hiding this comment.
With pagination/search, the existing "Dynamic Content Loader" (DOMContentLoaded) logic later in this template can conflict with the new no-results/pagination rendering: when URL params yield 0 matches on initial load, renderPage() shows #no-results, but the DOMContentLoaded handler will see 0 .article-card elements and overwrite .articles-grid with a .no-articles paragraph. This can lead to duplicate/confusing empty states. Remove/disable that handler for news indexes (or make it aware of filteredArticles/#no-results).
styles.css
Outdated
| .news-page .filter-group input[type="search"]:hover, | ||
| .news-page .filter-group input[type="search"]:focus { | ||
| border-color: var(--primary-cyan, #00d9ff); | ||
| outline: none; | ||
| } | ||
|
|
There was a problem hiding this comment.
The search input CSS removes the default focus outline (outline: none) on :focus without providing an equivalent visible focus indicator. This can fail WCAG 2.1 focus visibility expectations for keyboard users. Prefer using :focus-visible with an explicit outline (similar to .load-more-btn:focus-visible) and avoid removing focus styling entirely.
| .news-page .filter-group input[type="search"]:hover, | |
| .news-page .filter-group input[type="search"]:focus { | |
| border-color: var(--primary-cyan, #00d9ff); | |
| outline: none; | |
| } | |
| .news-page .filter-group input[type="search"]:hover { | |
| border-color: var(--primary-cyan, #00d9ff); | |
| } | |
| .news-page .filter-group input[type="search"]:focus-visible { | |
| border-color: var(--primary-cyan, #00d9ff); | |
| outline: 2px solid var(--primary-cyan, #00d9ff); | |
| outline-offset: 2px; | |
| } |
| <div class="filter-group search-group"> | ||
| <label for="search-input">${escapeHtml(lang.i18n.search)}</label> | ||
| <input type="search" id="search-input" placeholder="${escapeHtml(lang.i18n.searchPlaceholder)}" aria-label="${escapeHtml(lang.i18n.search)}" autocomplete="off"> |
There was a problem hiding this comment.
There are existing unit tests for news index generation, but the newly introduced client-side behaviors (search input, load-more/counter, and URL param keys such as q/type/topic/sort) aren’t currently asserted. Adding a few HTML-string assertions in tests/generate-news-indexes.test.ts would help prevent regressions (e.g., ensure #search-input, #load-more-btn, PAGE_SIZE, and readURLParams()/updateURL() appear in generated pages).
|
@copilot apply changes based on the comments in this thread |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
…ocus CSS, remove conflicting handler, add tests - Use JSON.stringify for i18nLoadMore to avoid HTML entities in JS aria-label - readURLParams() now validates select params against existing option values - Remove conflicting DOMContentLoaded handler that could overwrite pagination state - Fix search input CSS: use :focus-visible with explicit outline instead of outline:none - Modernize safeSetSelect to use const/let and arrow functions - Add 5 tests: search input, load-more/counter, pagination, URL state, no DOMContentLoaded Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all 5 review comments in commit
|
github-actions
bot
added
testing
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| <h2 class="article-title"> | ||
| <a href="\${article.slug}">\${article.title}</a> | ||
| </h2> | ||
| <p class="article-excerpt">\${article.excerpt}</p> | ||
| \${availableDisplay} | ||
| <div class="article-tags"> | ||
| \${article.tags.map(tag => \`<span class="tag">\${tag}</span>\`).join('')} | ||
| </div> |
There was a problem hiding this comment.
buildArticleCard() interpolates article.title, article.excerpt, article.tags, and article.slug directly into an HTML string that is assigned via innerHTML. Because these values are parsed from article HTML meta/title fields (see parseArticleMetadata()), this can break markup on characters like </& and also creates an XSS injection surface if article content is ever compromised. Consider escaping these fields before insertion (or build DOM nodes and set textContent/setAttribute instead of using innerHTML).
Fixed in commit |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| <h2 class="article-title"> | ||
| <a href="\${esc(article.slug)}">\${esc(article.title)}</a> | ||
| </h2> |
There was a problem hiding this comment.
esc() prevents HTML injection, but it does not prevent href-based XSS if article.slug ever contains a URL scheme (e.g. javascript:) because the browser will still treat it as a script URL even when escaped. Consider forcing generated links to be relative (e.g. prefixing with ./ or validating/rejecting slug values containing :///) before interpolating into the href attribute.
|
@copilot apply changes based on the comments in this thread |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
- Add safeHref() helper to generated JS that rejects any slug containing a protocol scheme (e.g. javascript:) or absolute path separators, falling back to '#' for unsafe values - Replace esc(article.slug) with safeHref(article.slug) in the href attribute of the article link - Update test to assert safeHref() is present and used for hrefs Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Fixed in commit |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| // Dynamic articles array - generated from news/ directory | ||
| const articles = ${JSON.stringify(displayData, null, 2)}; | ||
|
|
There was a problem hiding this comment.
displayData is embedded directly into an inline <script> via JSON.stringify(displayData, null, 2). If any article field ever contains the substring </script> (e.g., via a crafted <title>/og:title), the browser will terminate the script tag early and allow HTML/JS injection before esc() ever runs. Please serialize this data in a script-safe way (e.g., escape </</script in the JSON output, or place JSON in a <script type="application/json"> element and read textContent).
There was a problem hiding this comment.
Fixed in ff04ffb. The JSON.stringify(displayData, null, 2) call now chains .replace(/<\//g, '<\\/'), replacing every </ with <\/ in the serialized JSON. This prevents any </script> substring in article titles/excerpts from prematurely closing the script tag. At runtime the JS string value is unchanged since \/ evaluates as /.
| // Reject any value that looks like an absolute/protocol URL to prevent href-based XSS | ||
| if (/[:/]/.test(s.split('?')[0].split('#')[0]) || /^[a-zA-Z][a-zA-Z0-9+\-.]*:/.test(s)) { | ||
| return '#'; | ||
| } |
There was a problem hiding this comment.
safeHref() currently blocks : and /, but it still allows backslashes and other characters that browsers may normalize in surprising ways (e.g., \\example.com can be treated similarly to //example.com in some URL parsers). Consider tightening validation to an explicit allowlist for expected slugs (e.g., only [A-Za-z0-9._-] plus a required .html suffix) and rejecting \/control characters as well, instead of relying on the current substring checks.
| // Reject any value that looks like an absolute/protocol URL to prevent href-based XSS | |
| if (/[:/]/.test(s.split('?')[0].split('#')[0]) || /^[a-zA-Z][a-zA-Z0-9+\-.]*:/.test(s)) { | |
| return '#'; | |
| } | |
| // Only allow simple relative HTML filenames to avoid URL parsing/normalization issues. | |
| // Reject empty strings, backslashes, and control characters outright. | |
| if (!s || /[\\\x00-\x1F\x7F]/.test(s)) { | |
| return '#'; | |
| } | |
| // Allow only [A-Za-z0-9._-] characters with a required ".html" suffix. | |
| if (!/^[A-Za-z0-9._-]+\.html$/.test(s)) { | |
| return '#'; | |
| } |
There was a problem hiding this comment.
Fixed in ff04ffb. safeHref() now uses an explicit allowlist: rejects empty strings, backslashes, and control characters (/[\\\x00-\x1F\x7F]/), then requires the slug to match /^[A-Za-z0-9._-]+\.html$/ exactly — backslashes and any non-HTML filename form return #. This also fixes the ESLint no-useless-escape CI failure.
tests/generate-news-indexes.test.ts
Outdated
| expect(enContent).toContain('aria-live="polite"'); | ||
| // load-more must use addEventListener, not an inline onclick handler | ||
| expect(enContent).not.toContain('onclick="loadMore()"'); | ||
| expect(enContent).toContain("getElementById('load-more-btn').addEventListener('click', loadMore)"); |
There was a problem hiding this comment.
This test asserts an exact JS source substring for the addEventListener call, which makes the test brittle to harmless formatting/minification changes (quote style, whitespace, document. prefix, etc.). Prefer a regex or a less exact assertion (e.g., check that load-more-btn exists and that the generated script contains both addEventListener and loadMore in proximity).
| expect(enContent).toContain("getElementById('load-more-btn').addEventListener('click', loadMore)"); | |
| expect(enContent).toMatch(/addEventListener\s*\(\s*['"]click['"]\s*,\s*loadMore\s*\)/); |
There was a problem hiding this comment.
Fixed in ff04ffb. Changed from toContain(...) with a hardcoded string to toMatch(/addEventListener\s*\(\s*['"]click['"]\s*,\s*loadMore\s*\)/) as suggested.
|
@copilot apply changes based on the comments in this thread and Quality Checks / typescript-lint (pull_request)Failing after 37s |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
…gex in test Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
All three review comments addressed and the ESLint CI failure fixed in commit
All 34 tests pass, ESLint clean. |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| const flag = LANGUAGE_FLAGS[article.lang] || '🌐'; | ||
| const langBadge = \`<span class="language-badge" aria-label="\${esc(article.lang)} language"><span aria-hidden="true">\${flag}</span> \${esc(article.lang.toUpperCase())}</span>\`; | ||
|
|
||
| // Generate available languages display if multiple languages exist | ||
| const availableLangs = article.availableLanguages || [article.lang]; | ||
| let availableDisplay = ''; | ||
| if (availableLangs.length > 1) { | ||
| const availableBadges = availableLangs.map(l => { | ||
| const lf = LANGUAGE_FLAGS[l] || '🌐'; | ||
| return \`<span class="lang-badge-sm"><span aria-hidden="true">\${lf}</span> \${esc(l.toUpperCase())}</span>\`; | ||
| }).join(' '); | ||
| availableDisplay = \`<p class="available-languages"><strong>\${AVAILABLE_IN_TEXT}:</strong> \${availableBadges}</p>\`; |
There was a problem hiding this comment.
buildArticleCard() renders language badges without dir="ltr". On RTL pages (ar/he), language codes like EN/SV can be displayed in the wrong direction. There is already a repo pattern for this in generateLanguageBadge() (adds dir="ltr" when isRTL), so the client-side badges should apply the same RTL handling (both the main language-badge and the smaller lang-badge-sm badges).
| if (filteredArticles.length === 0) { | ||
| grid.innerHTML = ''; | ||
| noResults.style.display = 'block'; | ||
| if (counter) counter.textContent = ''; | ||
| if (btn) btn.style.display = 'none'; | ||
| return; |
There was a problem hiding this comment.
renderPage() uses the no-results message whenever filteredArticles.length === 0, which also covers the "no articles exist at all" case. That makes the UI misleading ("No articles matched the filters") and leaves lang.i18n.noArticles unused. Consider distinguishing between articles.length === 0 (show i18n.noArticles) vs. filteredArticles.length === 0 (show lang.noResults).
search,searchPlaceholder,loadMore,showingfields toI18nStringsintypes.tsconstants.tstemplate.ts: search input, 20-article pagination + "Load more", debounced search, URL state management, WCAG 2.1 AA ARIA labels and focus managementstyles.cssJSON.stringifyfori18nLoadMoreto avoid HTML entities in JS (fixes French apostrophe bug)readURLParams()validates URL params against allowed select options before applyingDOMContentLoadedhandler that could overwrite pagination/search state:focus-visiblewith explicit outline (nooutline: none)buildArticleCard: addesc()helper and escape all article fields beforeinnerHTMLinterpolationsafeHref()helper — now uses strict allowlist[A-Za-z0-9._-]+\.html$(rejects backslashes, control chars, non-html slugs); fixes ESLintno-useless-escapeCI failuredisplayDataJSON embedding: escape</as<\/to prevent</script>in article data from breaking the script tagonclick="loadMore()"withaddEventListener('click', loadMore)to avoid inline handler / CSP issuesaddEventListenertest assertion to use regex/addEventListener\s*\(\s*['"]click['"]\s*,\s*loadMore\s*\)/Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.