feat(xref): allow empty-term searches when specs are provided - #497
feat(xref): allow empty-term searches when specs are provided#497marcoscaceres wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for “browse” queries in xref search so callers can omit the search term when constraining by spec (and optionally filtering by type/for), with a safety cap on returned entries and new unit tests.
Changes:
- Allow
termto be omitted on the GET endpoint whenspecsare provided, defaultingtermto"". - Add a search fallback path for empty
term+specsthat aggregates entries by spec and applies type/for filtering with a 1000-result cap. - Add test fixtures and new test cases covering empty-term browsing scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| routes/xref/lib/search.ts | Implements empty-term/spec browsing via collectBySpecs() and adds a 1000-result cap. |
| routes/xref/search.get.ts | Makes term optional for GET requests when specs are present and defaults missing term to empty string. |
| tests/routes/xref/lib/search.test.js | Adds tests for empty-term browsing and expands cache clearing between suites. |
| tests/routes/xref/lib/data-by-spec.js | Introduces a bySpec fixture used by the new browsing tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
af53c85 to
d1fb2ac
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d1fb2ac to
4049d8e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot can you address the unresolved comments? |
Addressed in bc5238e:
|
|
@copilot Can you review this and fix any issues that come up? |
Done in f7fa003. Two remaining issues were addressed:
|
|
@copilot Can you confirm that all your comments have been addressed? Please resolve them if you’re happy. Can you fix anything outstanding from your feedback that hasn’t been addressed or anything else you find? |
All review comments have been addressed across commits bc5238e and f7fa003. Here's the full status: Resolved by code changes:
Accepted design decisions (no code changes needed):
All 48 tests pass. |
Move the 1000-entry cap from filter() to searchOne(), after filterBySpecType and filterPreferLatestVersion have run. This ensures preferred entries (current over snapshot, latest version) are retained before the result is capped. Add clarifying comments that types-only browsing (no term, no specs) is deliberately unsupported, and improve the route validation message. Addresses Copilot review threads on PR #497.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… to production shape
- resolveSpecKey: iterate over specmap groups (current/snapshot) to find
entry.shortname, fixing broken store.specmap?.[spec] flat-map assumption
- data-by-spec.js: rekey all entries by series shortname (not versioned spec id)
to match production specs.json shape, merge svg2 into svg, merge css-cascade-3/4
into css-cascade
- search.test.js: add minimal specmap to test store stub matching production
{ [group]: { [specid]: { shortname, url, title } } } shape; add test that
verifies versioned spec ids (css-lists-3, web-bluetooth-1) resolve via specmap
Agent-Logs-Url: https://github.com/speced/respec-web-services/sessions/dc6faf8f-7ab5-4633-a750-dc884e1aa53c
- filter(): apply .slice(0, 1000) on the empty-term browse result before returning so the MemCache stores a bounded list (not the full unbounded DataEntry[]), fixing the memory safety concern - searchOne(): remove the duplicate post-cache slice(0,1000) guards that were the only cap, now redundant since filter() is already bounded - search.get.ts: remove !types?.length from the 400-guard so that ?type=... without specs or term returns a proper 400 instead of silently falling through to an empty-string term search that returns misleading results; update error message accordingly Agent-Logs-Url: https://github.com/speced/respec-web-services/sessions/eafb7347-faf9-4e79-a820-4546c4b7b010
- Remove required attribute from term input - Allow form submission when specs are provided without a term - Use entry.term for cite generation when search term is empty - Include 'term' in requested fields so browse results have term data - Fall back to entry.spec for shortname display - Auto-open Advanced Search when URL has specs/types/for params - Remove options.includes guard from select() so URL-driven selections work before metadata options are loaded
Move the 1000-entry cap from filter() to searchOne(), after filterBySpecType and filterPreferLatestVersion have run. This ensures preferred entries (current over snapshot, latest version) are retained before the result is capped. Add clarifying comments that types-only browsing (no term, no specs) is deliberately unsupported, and improve the route validation message. Addresses Copilot review threads on PR #497.
6c7a322 to
b74f590
Compare
In browse mode a returned entry's spec may not be present in the client-side metadata.specs; guard with optional chaining and skip it instead of throwing, which previously blanked the entire results table.
The outer describe already clears the cache before every spec, so the five nested beforeEach(() => cache.clear()) hooks were redundant.
| async function handleSubmit() { | ||
| const data = getFormData(); | ||
| if (data.term === '') return; | ||
| if (data.term === '' && !data.specs) return; |
There was a problem hiding this comment.
Do we need a check for when term == '' vs typeof term === 'undefined'?
Summary
collectBySpecs()aggregates entries, applies type/for filtersCloses #278
Closes #208
Landing order: first of the xref trio that all edit
static/xref/script.js(#497, #529, #511) — land this, then #529 and #511 rebase onto main. Suggested batch order: #499, #500, #501, #497, #529, #511 (#424 independent).