Skip to content

Conversation

@yashuatla
Copy link
Owner

@yashuatla yashuatla commented Jun 24, 2025

PR Summary

Refactor Schema Validation and Add Head Tag Testing

Overview

This PR refactors schema validation components and adds comprehensive testing for head tag functionality. Key changes include converting the social links schema to an array-based approach, improving icon validation, and adding utilities for head tag testing.

Change Types

Type Description
Refactor Convert social links schema to array-based approach with migration support
Refactor Improve icon validation using dedicated schema
Enhancement Add new head tag testing functionality
Test Add comprehensive test file for head tag handling

Affected Modules

Module / File Change Description
starlight/schemas/social.ts Refactor from object-based to array-based approach with migration support
starlight/schemas/icon.ts Create new icon schema validation utility
starlight/schemas/hero.ts Replace enum validation with IconSchema()
starlight/utils/head.ts Add getHead function and modify createHead to internal
starlight/__tests__/head/head.test.ts Add new test file for head tag functionality
starlight/__tests__/test-utils.ts Add RouteDataContext type and utility function
starlight/__tests__/basics/starlight-page-route-data.test.ts Update test parameter to use context instead of URL

Notes for Reviewers

  • The social links schema has been completely refactored from object-based to array-based with migration support
  • The head tag functionality has been modified with createHead now being internal and a new getHead function added

HiDeoo and others added 15 commits April 7, 2025 11:37
…stro#2727)

Co-authored-by: Chris Swithinbank <[email protected]>
Co-authored-by: trueberryless <[email protected]>
Co-authored-by: SnowDingo <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Chris Swithinbank <[email protected]>
},
{ tag: 'title', content: `${data.title} ${config.titleDelimiter} ${siteTitle}` },
{ tag: 'link', attrs: { rel: 'canonical', href: canonicalHref } },
{ tag: 'meta', attrs: { name: 'generator', content: context.generator } },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Correctness Issue

Duplicate meta generator tags.

Two meta tags with name='generator' are defined with different content values, which violates HTML standards and causes unpredictable behavior across browsers.

Current Code (Diff):

- 		{ tag: 'meta', attrs: { name: 'generator', content: context.generator } },
- 		{
- 			tag: 'meta',
- 			attrs: { name: 'generator', content: `Starlight v${version}` },
- 		},
+ 		{ tag: 'meta', attrs: { name: 'generator', content: `${context.generator} (Starlight v${version})` } },
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
{ tag: 'meta', attrs: { name: 'generator', content: context.generator } },
// Use a single generator tag with combined information
{ tag: 'meta', attrs: { name: 'generator', content: `${context.generator} (Starlight v${version})` } },

tag: 'meta',
attrs: {
name: 'twitter:site',
content: new URL(twitterLink.href).pathname.replace('/', '@'),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Correctness Issue

Unsafe Twitter handle extraction.

The code assumes Twitter URLs always have a direct username path, but this fails with other URL formats like twitter.com/i/status/... or twitter.com/hashtag/...

Current Code (Diff):

- 					content: new URL(twitterLink.href).pathname.replace('/', '@'),
+ 					content: twitterLink.href.includes('twitter.com/') || twitterLink.href.includes('x.com/') ? 
+ 						'@' + new URL(twitterLink.href).pathname.split('/').filter(Boolean)[0] : 
+ 						new URL(twitterLink.href).hostname,
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
content: new URL(twitterLink.href).pathname.replace('/', '@'),
content: twitterLink.href.includes('twitter.com/') || twitterLink.href.includes('x.com/') ?
'@' + new URL(twitterLink.href).pathname.split('/').filter(Boolean)[0] :
new URL(twitterLink.href).hostname,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants