fix: handle missing title with titleTemplate and add defaultTitle#682
Closed
harlan-zw wants to merge 1 commit into
Closed
fix: handle missing title with titleTemplate and add defaultTitle#682harlan-zw wants to merge 1 commit into
harlan-zw wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client/API
participant Plugin as TemplateParamsPlugin
participant Resolver as resolveTitleTemplate
participant DOM as DOM/SSR Renderer
App->>Plugin: push head with titleTemplate + templateParams(defaultTitle)
Plugin->>Plugin: extract defaultTitle, compute rawPageTitle
Plugin->>Resolver: call processTemplateParams(rawPageTitle, params)
Resolver->>Resolver: compute substituted title (v)
alt rawPageTitle empty and defaultTitle present
Resolver->>DOM: set <title> textContent = defaultTitle
else v is null or unsafe to promote
Resolver-->>DOM: do not promote titleTemplate (no <title>)
else
Resolver->>DOM: set/promote <title> with substituted v
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Bundle Size Analysis
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/unhead/src/plugins/templateParams.ts (1)
17-39:⚠️ Potential issue | 🟠 MajorResolve
defaultTitleearlier and synthesize a title when none exists.This only mutates
tagMap.get('title'), so the fallback is skipped when there is no concretetitlekey yet — for exampletemplateParams.defaultTitlewithout anytitleTemplate, or a template-only title that was promoted fromtitleTemplatebut is still stored under thetitleTemplatekey. Because this runs intags:resolve,tags:beforeResolveconsumers also still observehead._titleas empty.defaultTitleneeds to be applied in the main title-resolution path, or at least beforetags:beforeResolve, and it should create/update the actual title tag instead of relying ontagMap.get('title').Possible direction
- 'tags:resolve': ({ tagMap, tags }) => { + 'tags:resolve': ({ tagMap, tags }) => { // ... const useDefaultTitle = !rawPageTitle && !!defaultTitle if (useDefaultTitle) { - const titleTag = tagMap.get('title') - if (titleTag) - titleTag.textContent = defaultTitle! + const titleTag = tags.find(tag => tag.tag === 'title') + if (titleTag) { + titleTag.textContent = defaultTitle! + } + else { + tags.push({ tag: 'title', props: {}, textContent: defaultTitle! }) + } + params.pageTitle = defaultTitle! + head._title = defaultTitle! }If the fallback must be visible to
tags:beforeResolveplugins too, this should move intopackages/unhead/src/utils/resolve.tsinstead of staying intags:resolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/unhead/src/plugins/templateParams.ts` around lines 17 - 39, The current tags:resolve handler applies defaultTitle only by mutating tagMap.get('title') which skips cases where no concrete title tag exists or when title is still represented by titleTemplate/head._title; move the defaultTitle logic earlier into the main title-resolution path (or into resolve.ts if tags:beforeResolve consumers must see it) so that when defaultTitle is present and rawPageTitle is empty you synthesize the actual title value into head._title and/or create/update the real title tag in tagMap (instead of only touching titleTag), ensuring the code paths that use processTemplateParams, tagMap, head._title and titleTemplate observe the fallback consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/unhead/src/plugins/templateParams.ts`:
- Around line 17-39: The current tags:resolve handler applies defaultTitle only
by mutating tagMap.get('title') which skips cases where no concrete title tag
exists or when title is still represented by titleTemplate/head._title; move the
defaultTitle logic earlier into the main title-resolution path (or into
resolve.ts if tags:beforeResolve consumers must see it) so that when
defaultTitle is present and rawPageTitle is empty you synthesize the actual
title value into head._title and/or create/update the real title tag in tagMap
(instead of only touching titleTag), ensuring the code paths that use
processTemplateParams, tagMap, head._title and titleTemplate observe the
fallback consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2698943-56fb-4e9b-a7b7-65e737933712
📒 Files selected for processing (8)
packages/unhead/src/plugins/templateParams.tspackages/unhead/src/types/schema/head.tspackages/unhead/src/types/tags.tspackages/unhead/src/utils/resolve.tspackages/unhead/test/unit/client/templateParams.test.tspackages/unhead/test/unit/client/titleTemplate.test.tspackages/unhead/test/unit/server/templateParams.test.tspackages/unhead/test/unit/server/titleTemplate.test.ts
…port When using a string titleTemplate like `%s - MyApp` without a page title, the result was an empty substitution (` - MyApp`). This commit: - Skips promoting `titleTemplate` to `title` when no page title is set and the template is a plain string containing `%s` (non-plugin path) - Adds `defaultTitle` to `TemplateParams` / `ResolvableTemplateParams`: when using the TemplateParamsPlugin, setting `templateParams.defaultTitle` causes that value to be used as the final title when no page-specific title is set, bypassing the titleTemplate entirely (e.g. `defaultTitle: 'MyApp'` with no title gives `<title>MyApp</title>` instead of `<title> - MyApp</title>`) Closes #618
harlan-zw
force-pushed
the
fix/618-default-title-support
branch
from
bf88354 to
4686abb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/unhead/test/unit/client/templateParams.test.ts (1)
250-264: Strengthen this test to actually catch the%sempty-substitution regressionOn Line 256,
%separator+%siteNamecan still produce<title>My Site</title>without usingdefaultTitle, so this case may pass even if the new behavior is broken.Suggested test adjustment
it('defaultTitle used when no page title is set', async () => { const head = useDOMHead({ plugins: [TemplateParamsPlugin], }) useHead(head, { - titleTemplate: '%s %separator %siteName', + titleTemplate: '%s - My App', templateParams: { - siteName: 'My Site', defaultTitle: 'My Site', }, }) - expect(await useDelayedSerializedDom()).toContain('<title>My Site</title>') + const html = await useDelayedSerializedDom() + expect(html).toContain('<title>My Site</title>') + expect(html).not.toContain('<title> - My App</title>') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/unhead/test/unit/client/templateParams.test.ts` around lines 250 - 264, The test currently can pass even when %s is not replaced by defaultTitle because %separator + %siteName can produce the same output; update the test that uses useDOMHead/TemplateParamsPlugin and useHead so templateParams.siteName is a different value than defaultTitle (e.g., 'Other Site') and set templateParams.defaultTitle to 'My Site', then assert the serialized DOM contains exactly '<title>My Site</title>' — this ensures the code must substitute defaultTitle for the empty %s instead of relying on %separator/%siteName producing that string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/unhead/src/plugins/templateParams.ts`:
- Around line 35-39: When useDefaultTitle is true we currently only set
defaultTitle if an existing titleTag is present, so inputs that only provide
titleTemplate still end up producing %s-based titles; fix by ensuring a title
tag is created or populated when absent: in the same block that checks
useDefaultTitle and accesses tagMap/get('title'), if titleTag is undefined
create a new title tag entry in tagMap (or otherwise set the resolved title
value) and set its textContent to defaultTitle; also ensure this override
happens before any titleTemplate resolution so defaultTitle always wins when
useDefaultTitle is true. Use the symbols useDefaultTitle, tagMap, titleTag,
defaultTitle and titleTemplate to locate and modify the logic.
---
Nitpick comments:
In `@packages/unhead/test/unit/client/templateParams.test.ts`:
- Around line 250-264: The test currently can pass even when %s is not replaced
by defaultTitle because %separator + %siteName can produce the same output;
update the test that uses useDOMHead/TemplateParamsPlugin and useHead so
templateParams.siteName is a different value than defaultTitle (e.g., 'Other
Site') and set templateParams.defaultTitle to 'My Site', then assert the
serialized DOM contains exactly '<title>My Site</title>' — this ensures the code
must substitute defaultTitle for the empty %s instead of relying on
%separator/%siteName producing that string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d3d74c8-ef65-44c9-b882-997cb06099fa
📒 Files selected for processing (8)
packages/unhead/src/plugins/templateParams.tspackages/unhead/src/types/schema/head.tspackages/unhead/src/types/tags.tspackages/unhead/src/utils/resolve.tspackages/unhead/test/unit/client/templateParams.test.tspackages/unhead/test/unit/client/titleTemplate.test.tspackages/unhead/test/unit/server/templateParams.test.tspackages/unhead/test/unit/server/titleTemplate.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/unhead/test/unit/server/titleTemplate.test.ts
- packages/unhead/test/unit/server/templateParams.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/unhead/test/unit/client/titleTemplate.test.ts
- packages/unhead/src/types/schema/head.ts
- packages/unhead/src/types/tags.ts
- packages/unhead/src/utils/resolve.ts
Comment on lines
+35
to
+39
| if (useDefaultTitle) { | ||
| const titleTag = tagMap.get('title') | ||
| if (titleTag) | ||
| titleTag.textContent = defaultTitle! | ||
| } |
There was a problem hiding this comment.
defaultTitle does not reliably bypass titleTemplate
On Line 36, this only applies when a title tag already exists. If the input only has titleTemplate, this branch is a no-op, and later title resolution can still render %s-based output (e.g., - MyApp) instead of defaultTitle.
Suggested fix
const useDefaultTitle = !rawPageTitle && !!defaultTitle
if (useDefaultTitle) {
- const titleTag = tagMap.get('title')
- if (titleTag)
- titleTag.textContent = defaultTitle!
+ // enforce bypass of titleTemplate when defaultTitle is used
+ tagMap.delete('titleTemplate')
+ const titleTag = tagMap.get('title')
+ if (titleTag) {
+ tagMap.set('title', { ...titleTag, textContent: defaultTitle! })
+ }
+ else {
+ tagMap.set('title', { tag: 'title', textContent: defaultTitle! } as HeadTag)
+ }
}📝 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.
Suggested change
| if (useDefaultTitle) { | |
| const titleTag = tagMap.get('title') | |
| if (titleTag) | |
| titleTag.textContent = defaultTitle! | |
| } | |
| if (useDefaultTitle) { | |
| // enforce bypass of titleTemplate when defaultTitle is used | |
| tagMap.delete('titleTemplate') | |
| const titleTag = tagMap.get('title') | |
| if (titleTag) { | |
| tagMap.set('title', { ...titleTag, textContent: defaultTitle! }) | |
| } | |
| else { | |
| tagMap.set('title', { tag: 'title', textContent: defaultTitle! } as HeadTag) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/unhead/src/plugins/templateParams.ts` around lines 35 - 39, When
useDefaultTitle is true we currently only set defaultTitle if an existing
titleTag is present, so inputs that only provide titleTemplate still end up
producing %s-based titles; fix by ensuring a title tag is created or populated
when absent: in the same block that checks useDefaultTitle and accesses
tagMap/get('title'), if titleTag is undefined create a new title tag entry in
tagMap (or otherwise set the resolved title value) and set its textContent to
defaultTitle; also ensure this override happens before any titleTemplate
resolution so defaultTitle always wins when useDefaultTitle is true. Use the
symbols useDefaultTitle, tagMap, titleTag, defaultTitle and titleTemplate to
locate and modify the logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
Resolves #618
❓ Type of change
📚 Description
When
titleTemplate: '%s - MyApp'was set with no page title,%swas replaced with empty string producing' - MyApp'. Now the title tag is silently dropped when no title exists and the template contains%s. Additionally, a newdefaultTitleoption intemplateParamsprovides React Helmet-style behavior — when no page title is set anddefaultTitleis provided, it renders as the title bypassing the template entirely.Summary by CodeRabbit
New Features
defaultTitletemplate parameter that serves as a fallback page title when no page title is provided, preventing partially rendered title templates (e.g., "%s - My Site") from producing incomplete results.Bug Fixes
defaultTitleor empty title behavior is handled predictably.Tests
defaultTitleand edge-case titleTemplate behaviors.