feat: metadata parity for appLinks, iTunes, Twitter player/app cards#452
Conversation
…pp cards Add four missing Next.js metadata features to the Metadata type and MetadataHead renderer: - appLinks: renders al:* property tags (ios, iphone, ipad, android, windows_phone, windows, windows_universal, web) - itunes: renders apple-itunes-app name tag with app-id and app-argument - twitter player cards: renders twitter:player, twitter:player:stream, twitter:player:width, twitter:player:height - twitter app cards: renders twitter:app:name/id/url per platform (iphone, ipad, googleplay) Includes fixture pages and tests ported from Next.js test suite.
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd7dd95a8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <meta key={key++} name="twitter:player" content={player.playerUrl.toString()} />, | ||
| ); | ||
| elements.push( | ||
| <meta key={key++} name="twitter:player:stream" content={player.streamUrl.toString()} />, |
There was a problem hiding this comment.
The new Twitter player/app rendering path emits URL values via .toString() instead of the existing resolveUrl helper, so relative values (for example playerUrl: "/player" with metadataBase) are output as relative strings instead of absolute URLs. This is inconsistent with how other metadata URLs are handled in this file (e.g. OG/Twitter images) and causes malformed Twitter card metadata unless every caller manually provides absolute URLs.
Useful? React with 👍 / 👎.
| property={`al:${platform}:${k}`} | ||
| content={String(v)} | ||
| />, |
There was a problem hiding this comment.
App Links tags currently stringify descriptor values directly, so url fields under appLinks (such as al:ios:url or al:web:url) remain relative when metadata uses metadataBase. Because these tags are expected to carry canonical URLs and the shim already resolves other URL-bearing metadata fields, this introduces incorrect output for relative-path configurations.
Useful? React with 👍 / 👎.
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: metadata parity for appLinks, iTunes, Twitter player/app cards
Good work filling in the four documented metadata gaps. The types match Next.js definitions, the fixture pages and tests are well-structured, and the rendering logic is correct for the common cases. I have one behavioral difference from Next.js and a few minor nits.
Behavioral difference
Twitter app card name emission is gated on id presence — Next.js doesn't do this. In Next.js, the name, id, and url checks inside the platform loop are independent. If app.name is set, Next.js emits twitter:app:name:{platform} for all three platforms regardless of whether app.id[platform] exists. The current implementation nests name inside if (app.id[platform] !== undefined), so a platform without an id won't get a name tag either. See inline comment below.
This probably doesn't matter in practice (you'd rarely set name without any id), but since the goal is Next.js parity, it's worth aligning.
Test coverage note
The twitter-app test fixture happens to have all three platforms with both id and name set, so it doesn't exercise the divergence above. A test with name set but only one platform's id defined would catch it.
Summary
Overall this is a clean, well-tested PR. The one structural issue (twitter app name gating) is a minor parity gap. The rest looks solid — types are accurate, resolveUrl is used correctly for URL fields, and the N/A comments are properly cleaned up.
| for (const platform of ["iphone", "ipad", "googleplay"] as const) { | ||
| if (app.id[platform] !== undefined) { | ||
| if (app.name) { | ||
| elements.push( | ||
| <meta key={key++} name={`twitter:app:name:${platform}`} content={app.name} />, | ||
| ); | ||
| } | ||
| elements.push( | ||
| <meta | ||
| key={key++} | ||
| name={`twitter:app:id:${platform}`} | ||
| content={String(app.id[platform])} | ||
| />, | ||
| ); | ||
| if (app.url?.[platform] !== undefined) { | ||
| const appUrl = app.url[platform]!.toString(); | ||
| elements.push( | ||
| <meta | ||
| key={key++} | ||
| name={`twitter:app:url:${platform}`} | ||
| content={resolveUrl(appUrl) ?? appUrl} | ||
| />, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Behavioral difference from Next.js: In Next.js, the name, id, and url checks inside the platform loop are independent — not nested. If app.name is set, Next.js emits twitter:app:name:{platform} for all three platforms regardless of whether app.id[platform] exists:
// Next.js (packages/next/src/lib/metadata/metadata.tsx)
for (const platform of ['iphone', 'ipad', 'googleplay'] as const) {
if (app.name) { /* emit name */ }
if (app.id[platform]) { /* emit id */ }
if (app.url?.[platform]) { /* emit url */ }
}Here, name and url are nested inside if (app.id[platform] !== undefined), which means a platform without an id won't get name or url tags. This diverges from Next.js behavior.
| for (const platform of ["iphone", "ipad", "googleplay"] as const) { | |
| if (app.id[platform] !== undefined) { | |
| if (app.name) { | |
| elements.push( | |
| <meta key={key++} name={`twitter:app:name:${platform}`} content={app.name} />, | |
| ); | |
| } | |
| elements.push( | |
| <meta | |
| key={key++} | |
| name={`twitter:app:id:${platform}`} | |
| content={String(app.id[platform])} | |
| />, | |
| ); | |
| if (app.url?.[platform] !== undefined) { | |
| const appUrl = app.url[platform]!.toString(); | |
| elements.push( | |
| <meta | |
| key={key++} | |
| name={`twitter:app:url:${platform}`} | |
| content={resolveUrl(appUrl) ?? appUrl} | |
| />, | |
| ); | |
| } | |
| } | |
| if (tw.app) { | |
| const { app } = tw; | |
| for (const platform of ["iphone", "ipad", "googleplay"] as const) { | |
| if (app.name) { | |
| elements.push( | |
| <meta key={key++} name={`twitter:app:name:${platform}`} content={app.name} />, | |
| ); | |
| } | |
| if (app.id[platform] !== undefined) { | |
| elements.push( | |
| <meta | |
| key={key++} | |
| name={`twitter:app:id:${platform}`} | |
| content={String(app.id[platform])} | |
| />, | |
| ); | |
| } | |
| if (app.url?.[platform] !== undefined) { | |
| const appUrl = app.url[platform]!.toString(); | |
| elements.push( | |
| <meta | |
| key={key++} | |
| name={`twitter:app:url:${platform}`} | |
| content={resolveUrl(appUrl) ?? appUrl} | |
| />, | |
| ); | |
| } | |
| } | |
| } |
| if (!entries) continue; | ||
| const list = Array.isArray(entries) ? entries : [entries]; | ||
| for (const entry of list) { | ||
| for (const [k, v] of Object.entries(entry)) { |
There was a problem hiding this comment.
Nit: Object.entries(entry) iterates properties in insertion order, which means the tag order depends on how the user wrote their object literal. This is fine functionally (and how Next.js does it too for resolved arrays), but worth noting that for AppLinksAndroid the package field will render as al:android:package — which is correct. Just double-checking you considered that Object.entries will include all enumerable own properties, so if someone passes unexpected extra fields they'd also be rendered. Low risk but worth being aware of.
| if (tw.players) { | ||
| const players = Array.isArray(tw.players) ? tw.players : [tw.players]; | ||
| for (const player of players) { | ||
| const playerUrl = player.playerUrl.toString(); |
There was a problem hiding this comment.
Minor: player.playerUrl.toString() is called before passing to resolveUrl, which is good. But note that if playerUrl is already a URL object, .toString() produces an absolute URL that resolveUrl will pass through unchanged (the startsWith('http') check). This means metadataBase resolution only kicks in for string values that are relative paths — which is the correct behavior and matches how it should work. Just confirming this is intentional.
| expect(html).toContain('name="twitter:app:name:iphone" content="My App"'); | ||
| expect(html).toContain('name="twitter:app:id:iphone" content="id123456789"'); | ||
| expect(html).toContain('name="twitter:app:url:iphone" content="https://example.com/iphone"'); | ||
| expect(html).toContain('name="twitter:app:id:ipad" content="id123456789"'); |
There was a problem hiding this comment.
The test doesn't assert twitter:app:name:ipad. Since the fixture sets name: "My App" and all three platforms have IDs, the tag should be present. Consider adding:
| expect(html).toContain('name="twitter:app:id:ipad" content="id123456789"'); | |
| expect(html).toContain('name="twitter:app:name:ipad" content="My App"'); | |
| expect(html).toContain('name="twitter:app:id:ipad" content="id123456789"'); |
|
Review posted successfully. Here's a summary of what I found: One behavioral difference from Next.js:
Minor items:
Overall, the PR is well-structured with clean types, good fixture pages, and solid test coverage. The main actionable item is the twitter app |
2 participants
Summary
appLinksmetadata support — rendersal:*property tags for iOS, Android, Windows, and web platformsitunesmetadata support — rendersapple-itunes-appmeta tag withapp-idandapp-argumenttwitter:player,twitter:player:stream,twitter:player:width,twitter:player:heighttwitter:app:name/id/urlper platform (iphone, ipad, googleplay)All four were documented as N/A gaps in
metadata.test.ts(lines 345, 354, 363). Types match Next.jsextra-types.d.tsandtwitter-types.d.tsdefinitions. Rendering matches Next.jsgenerate/opengraph.jsandgenerate/basic.jsoutput.Test plan
metadata-itunes,metadata-applinks,metadata-twitter-player,metadata-twitter-app)