perf(search): skip SSR for high-cardinality tag combos #1575
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis change optimizes the search results page's SSR (Server-Side Rendering) logic by introducing early extraction of search filters, cardinality-based browse detection, and a classification system for skipping SSR. It also enhances observability through richer logging that includes filter counts and skip reasons. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pages/q/`[[...all]].tsx:
- Around line 331-338: The instructorsCount cardinality check is inconsistent
with topicsCount and can count malformed 'undefined' strings; update the
instructorsCount calculation to mirror topicsCount by first checking
Array.isArray(selectedInstructors) and then filtering out falsy values and the
literal 'undefined' string before taking length (i.e., apply the same filter
logic used for selectedTopics). Locate and change the instructorsCount
definition that references selectedInstructors so it uses
selectedInstructors.filter(i => i && i !== 'undefined').length (with a fallback
of 0 when not an array).
🧹 Nitpick comments (1)
src/pages/q/[[...all]].tsx (1)
442-444: Nice observability improvement — minor naming note.The new fields give good visibility into the cardinality-based skip behavior. One small thing: the log field is
tags_countbut the variable istopicsCount. This works, but the mixed terminology (tagsin logs vstopicsin code) could trip up someone correlating log output with the source. Consider aligning on one term for both, or adding a brief comment.
| // Our `/q/<path>` URLs can encode multiple tags/instructors (combinatorial explosion). | ||
| // SSR'ing those pages is expensive and produces near-zero cache reuse. | ||
| const topicsCount = Array.isArray(selectedTopics) | ||
| ? selectedTopics.filter((t) => t && t !== 'undefined').length | ||
| : 0 | ||
| const instructorsCount = Array.isArray(selectedInstructors) | ||
| ? selectedInstructors.length | ||
| : 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find getInstructorsFromSearchState and topicExtractor implementations
rg -n "getInstructorsFromSearchState" --type ts --type tsx -B2 -A8
echo "---"
rg -n "topicExtractor" --type ts --type tsx -B2 -A8Repository: skillrecordings/egghead-next
Length of output: 148
🏁 Script executed:
# Check the file itself to understand context around lines 331-338
sed -n '320,360p' src/pages/q/[[...all]].tsxRepository: skillrecordings/egghead-next
Length of output: 1670
🏁 Script executed:
# Search for where selectedInstructors and selectedTopics are assigned
rg -n "selectedInstructors\s*=" src/pages/q/[[...all]].tsx -B3 -A1
echo "---"
rg -n "selectedTopics\s*=" src/pages/q/[[...all]].tsx -B3 -A1Repository: skillrecordings/egghead-next
Length of output: 576
🏁 Script executed:
# Find getInstructorsFromSearchState definition
rg -n "getInstructorsFromSearchState" --type ts -B2 -A10Repository: skillrecordings/egghead-next
Length of output: 2835
🏁 Script executed:
# Find topicExtractor definition
rg -n "topicExtractor" --type ts -B2 -A10Repository: skillrecordings/egghead-next
Length of output: 4676
🏁 Script executed:
# Find parseUrl to understand the data flow
rg -n "parseUrl" --type ts -B2 -A15 | head -100Repository: skillrecordings/egghead-next
Length of output: 7096
🏁 Script executed:
# Verify the full parseUrl implementation to understand data source
sed -n '131,210p' src/lib/search-url-builder.tsRepository: skillrecordings/egghead-next
Length of output: 1084
🏁 Script executed:
# Check if there are any tests showing 'undefined' strings in arrays
rg -n "'undefined'" src/lib/__tests__/search-url-builder.test.ts -C2Repository: skillrecordings/egghead-next
Length of output: 54
🏁 Script executed:
# Check tagsForPath and instructorsForPath to see if they produce 'undefined' strings
rg -n "tagsForPath|instructorsForPath" src/lib/search-url-builder.ts -B2 -A8Repository: skillrecordings/egghead-next
Length of output: 1547
🏁 Script executed:
# Check the actual implementation of these functions
rg -n "const tagsForPath|const instructorsForPath|function tagsForPath|function instructorsForPath" --type ts -A10Repository: skillrecordings/egghead-next
Length of output: 1460
🏁 Script executed:
# Check if nameSlugToName could produce 'undefined' strings
rg -n "nameSlugToName" src/lib/search-url-builder.ts -B2 -A5Repository: skillrecordings/egghead-next
Length of output: 884
🏁 Script executed:
# Find nameSlugToName definition
rg -n "export.*nameSlugToName|const nameSlugToName|function nameSlugToName" --type ts -A8Repository: skillrecordings/egghead-next
Length of output: 653
🏁 Script executed:
# Check if there are any client-side manipulations of selectedInstructors or instructor_name
rg -n "instructor_name|refinementList" src/pages/q/[[...all]].tsx -B1 -A1 | head -60Repository: skillrecordings/egghead-next
Length of output: 220
🏁 Script executed:
# Check if topicExtractor itself does any filtering on _tags
cat src/utils/search/topic-extractor.tsRepository: skillrecordings/egghead-next
Length of output: 781
Inconsistent filtering between topics and instructors in cardinality check.
topicsCount defensively filters out falsy values and the literal 'undefined' string (line 334), but instructorsCount takes the raw array length (line 336). Since topicExtractor can return unfiltered _tags arrays and the codebase handles 'undefined' strings from malformed URLs (see line 264), applying the same filter to instructorsCount ensures consistent cardinality detection.
Suggested fix
const instructorsCount = Array.isArray(selectedInstructors)
- ? selectedInstructors.length
+ ? selectedInstructors.filter((i) => i && i !== 'undefined').length
: 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Our `/q/<path>` URLs can encode multiple tags/instructors (combinatorial explosion). | |
| // SSR'ing those pages is expensive and produces near-zero cache reuse. | |
| const topicsCount = Array.isArray(selectedTopics) | |
| ? selectedTopics.filter((t) => t && t !== 'undefined').length | |
| : 0 | |
| const instructorsCount = Array.isArray(selectedInstructors) | |
| ? selectedInstructors.length | |
| : 0 | |
| // Our `/q/<path>` URLs can encode multiple tags/instructors (combinatorial explosion). | |
| // SSR'ing those pages is expensive and produces near-zero cache reuse. | |
| const topicsCount = Array.isArray(selectedTopics) | |
| ? selectedTopics.filter((t) => t && t !== 'undefined').length | |
| : 0 | |
| const instructorsCount = Array.isArray(selectedInstructors) | |
| ? selectedInstructors.filter((i) => i && i !== 'undefined').length | |
| : 0 |
🤖 Prompt for AI Agents
In `@src/pages/q/`[[...all]].tsx around lines 331 - 338, The instructorsCount
cardinality check is inconsistent with topicsCount and can count malformed
'undefined' strings; update the instructorsCount calculation to mirror
topicsCount by first checking Array.isArray(selectedInstructors) and then
filtering out falsy values and the literal 'undefined' string before taking
length (i.e., apply the same filter logic used for selectedTopics). Locate and
change the instructorsCount definition that references selectedInstructors so it
uses selectedInstructors.filter(i => i && i !== 'undefined').length (with a
fallback of 0 when not an array).
1 participant
Search URLs can encode multiple tags/instructors in the path (combinatorial explosion). SSR + caching those pages is expensive and yields near-zero reuse.
This changes
/q/[[...all]]SSR logic to only SSR/cache low-cardinality browse pages:Also adds structured fields to
search_ssr_cachelogs:tags_count,instructors_countskip_reason(low-cardinality enum)Refs: #1556
Summary by CodeRabbit