feat: set page title on viewed page #852
Conversation
…cumentTitle hooks that let us provide an unique title for pages inside AppRoutes
stanislavsemeniuk
requested review from
CryptoRodeo,
carlosthe19916 and
kahboom
Reviewer's GuideIntroduce a reusable DocumentMetadata component that sets the browser document title (including branding) and wire it into key pages so each route renders a page-specific title. Sequence diagram for setting document title via DocumentMetadatasequenceDiagram
actor User
participant Router
participant PageComponent
participant DocumentMetadata
participant useBranding
participant BrowserDocument
User->>Router: navigate to route
Router->>PageComponent: render()
PageComponent->>DocumentMetadata: render(title)
DocumentMetadata->>useBranding: getBranding()
useBranding-->>DocumentMetadata: branding (application title)
DocumentMetadata->>DocumentMetadata: compose documentTitle
DocumentMetadata-->>PageComponent: <title> element
PageComponent-->>Router: rendered tree
Router-->>BrowserDocument: update DOM
BrowserDocument->>BrowserDocument: set document.title to documentTitle
Class diagram for DocumentMetadata component and consuming pagesclassDiagram
class DocumentMetadata {
+string~optional~ title
+render()
-string baseTitle
-string documentTitle
}
class useBranding {
+getBranding()
+application
}
class Home {
+render()
}
class AdvisoryDetails {
+render()
}
class AdvisoryList {
+render()
}
class AdvisoryUpload {
+render()
}
class ImporterList {
+render()
}
class LicenseList {
+render()
}
class PackageDetails {
+render()
}
class PackageList {
+render()
}
class SbomDetails {
+render()
}
class SbomList {
+render()
}
class SbomScan {
+render()
}
class SbomUpload {
+render()
}
class Search {
+render()
}
class VulnerabilityDetails {
+render()
}
class VulnerabilityList {
+render()
}
DocumentMetadata ..> useBranding : uses
Home ..> DocumentMetadata : renders
AdvisoryDetails ..> DocumentMetadata : renders
AdvisoryList ..> DocumentMetadata : renders
AdvisoryUpload ..> DocumentMetadata : renders
ImporterList ..> DocumentMetadata : renders
LicenseList ..> DocumentMetadata : renders
PackageDetails ..> DocumentMetadata : renders
PackageList ..> DocumentMetadata : renders
SbomDetails ..> DocumentMetadata : renders
SbomList ..> DocumentMetadata : renders
SbomScan ..> DocumentMetadata : renders
SbomUpload ..> DocumentMetadata : renders
Search ..> DocumentMetadata : renders
VulnerabilityDetails ..> DocumentMetadata : renders
VulnerabilityList ..> DocumentMetadata : renders
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
useRouteDocumentTitle, you create a new array with[...matches].reverse()on every render; consider iterating from the end ofmatchesinstead to avoid the extra allocation and reversal on each render. - The cast to
MatchWithTitleHandle[]foruseMatches()is a bit loose and assumes all matches conform to that shape; you could narrow more safely (e.g., by checkingtypeof match.handle === 'object' && 'title' in match.handle) instead of casting the whole array.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useRouteDocumentTitle`, you create a new array with `[...matches].reverse()` on every render; consider iterating from the end of `matches` instead to avoid the extra allocation and reversal on each render.
- The cast to `MatchWithTitleHandle[]` for `useMatches()` is a bit loose and assumes all matches conform to that shape; you could narrow more safely (e.g., by checking `typeof match.handle === 'object' && 'title' in match.handle`) instead of casting the whole array.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #852 +/- ##
==========================================
- Coverage 61.83% 61.41% -0.42%
==========================================
Files 175 176 +1
Lines 3139 3144 +5
Branches 716 717 +1
==========================================
- Hits 1941 1931 -10
- Misses 928 952 +24
+ Partials 270 261 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
By the way, we can just use <title>...</title> inside components in react 19 , i just think to set it where the app routes are set is more structured approach but we can discuss that |
@stanislavsemeniuk thanks for the PR. Yeah, seems there are multiple paths we could take. I wonder how would we evolve the current approach if we need the title of the page to reflect the current object we are looking at. E.g. When we navigate to the details of a Vulnerability and we are in the Vulnerability Details Page then how to set the title of the page to something like Although the requirement in the ticket does not mention that, maybe it is worth evaluating such scenario. WDYT? |
|
@carlosthe19916 Yeah, i think the only way can pass some data that we got from BE into title is by using <title> inside components rendering. But i think we should also agree to have some "rule" of proper way of using <title> not to create mess in the project. How about use <title> only in components that are passed as route element, i mean this: Or if route element don't have data that is needed for <title> then implement it in the closest child possible Sounds right? |
|
@stanislavsemeniuk good point. |
|
@stanislavsemeniuk in regards of my previous comment. I have created a draft PR to validate the idea #853 . That PR is able to create titles like There might be other strategies, so let's evaluate them :) |
|
Just duplication comment from #853 @carlosthe19916 to be honest in some way it feels like overcomplicating things. And also there can be usage of loader without actual loading of data that feels like misconception (f.e in Advisory list) I think the other way to not make it overcomplicated and don't create mess of inside JSX everywhere at the same time could be something like this: Took SeoMetadata component that u implemented (or implement something similar), change it a bit to make it as a context provider that will have some setSeoMetadata func that can be used accross components for setting metadata including title. That would help us have a rendering of metadata stored only in one component and provide a simple func for changing it on different pages. What do you think? |
Good point
If we use a Context then whenever the value of the context changes it will re-render all child components. I wonder if we could use your proposal without the need of a Context and just use a standard component? E.g. const page1 = (
<>
<SeoComponent seoProperties={value}/>
...rest of page here
</>)this way the seo component lives only while the page lives and it does not force a tree hierarchy to be re-rendered This would still enforce your idea of having a single place where to define seo and yet each page can set its own values there, including the details page like Vulnerability Details Page |
|
@carlosthe19916 I was thinking about pages where there are tabs. and I think we should also change title when tabs are changed and that mean that we can't set title on some high level components and we should use it deeply inside the components. So the solution i have right now for it to use shared useDocumentTitle hook that lets us set up a proper title accroding to tab and even using data from BE. You can see example of usage in home.tsx , SearchTabs.tsx and vulnerability-details.tsx What do you think? |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
SearchTabs, the document title is derived fromgetTabsProps().activeKey, which appears to be an internal key (e.g.sboms,packages) rather than a user-facing label; consider using the tab's display label so the browser title is more readable. - In
VulnerabilityDetails,useDocumentTitle(vulnerability?.identifier ?? "")can result in an empty or flickering title while the data loads; consider providing a stable fallback (e.g."Vulnerability details") and then appending the identifier once available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SearchTabs`, the document title is derived from `getTabsProps().activeKey`, which appears to be an internal key (e.g. `sboms`, `packages`) rather than a user-facing label; consider using the tab's display label so the browser title is more readable.
- In `VulnerabilityDetails`, `useDocumentTitle(vulnerability?.identifier ?? "")` can result in an empty or flickering title while the data loads; consider providing a stable fallback (e.g. `"Vulnerability details"`) and then appending the identifier once available.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Overall this LGTM. I used the hook on the SBOMS list page and the SBOM details page, see demo: useDocumentTitleDemo.mp4Adding this functionality to a component is easy and if the component has the data we need it's easy to add additional information to the title. It looks like the current options discussed are:
I haven't taken a deep look at #853 yet, but I'm wondering if the data for a component can also be fetched from the |
|
@stanislavsemeniuk some thoughts:
<>
<DocumentTitle title={"something"} />
<PageSection>
// patternfly elements
</PageSection>
</>
Screencast.From.2026-01-07.09-11-53.mp4 |
…a, get rid of useDocumentTitle hook and titles in tabs
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Rendering a
<title>element inside the page JSX will not updatedocument.titleunless it’s mounted into<head>(e.g., viareact-helmetor a similar mechanism), so consider switchingDocumentMetadatato a hook that setsdocument.titleor integrating a head-management library. - You’re now manually adding
<DocumentMetadata>to each page component; since the PR description mentions route-level metadata, you might simplify this by deriving the title from routehandledata in a single place (e.g., a layout or router wrapper) rather than scattering title components across all pages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Rendering a `<title>` element inside the page JSX will not update `document.title` unless it’s mounted into `<head>` (e.g., via `react-helmet` or a similar mechanism), so consider switching `DocumentMetadata` to a hook that sets `document.title` or integrating a head-management library.
- You’re now manually adding `<DocumentMetadata>` to each page component; since the PR description mentions route-level metadata, you might simplify this by deriving the title from route `handle` data in a single place (e.g., a layout or router wrapper) rather than scattering title components across all pages.
## Individual Comments
### Comment 1
<location> `client/src/app/components/DocumentMetadata.tsx:3-12` </location>
<code_context>
+import useBranding from "@app/hooks/useBranding";
+
+export const DocumentMetadata = ({ title }: { title?: string }) => {
+ const branding = useBranding();
+ const baseTitle = branding.application.title;
+ const documentTitle = title ? `${title} | ${baseTitle}` : baseTitle;
+
+ return (
+ <>
+ <title>{documentTitle}</title>
+ </>
+ );
+};
</code_context>
<issue_to_address>
**issue (bug_risk):** Rendering a <title> element in the component tree is unlikely to update the document title correctly; consider setting document.title via an effect instead.
Rendering `<title>` inside the React tree (under `<body>`) is invalid HTML and usually won’t update the browser tab title in an SPA. Instead, compute the title here and set it via a side effect:
```tsx
import { useEffect } from "react";
import useBranding from "@app/hooks/useBranding";
export const DocumentMetadata = ({ title }: { title?: string }) => {
const branding = useBranding();
const baseTitle = branding.application.title;
const documentTitle = title ? `${title} | ${baseTitle}` : baseTitle;
useEffect(() => {
document.title = documentTitle;
}, [documentTitle]);
return null;
};
```
This avoids invalid markup and reliably updates the tab title wherever this component is used.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@carlosthe19916 @CryptoRodeo Updated pr. Implemented DocumentMetadata component and used new <title> JSX tag in it |
carlosthe19916
left a comment
carlosthe19916
left a comment
There was a problem hiding this comment.
@stanislavsemeniuk thanks for the enhancements. I tested it and all issues reported in my previous comments are gone, thanks!
Just one minor thing, I noticed that in github actions and also locally when i execute npm run check then the biome rules are throwing a warning about DocumentMedatada and suggesting to remove <></> . Could you do that please?
I wonder why CI and github actions didn't fail, i was expecting it to fail since our biome commands defined in packge.json has biome check --error-on-warnings so I was expecting even the warnings to thrown error. But that is something to investigate separately.
carlosthe19916
changed the title
carlosthe19916
changed the title
3 participants
This PR is trying to resolve this issue: https://issues.redhat.com/browse/TC-3370
Implemented useDocumentTitle & useRouteDocumentTitle hooks that give us an opportunity to provide a unique title to each page by simply providing
handle: {title : string | func}to each route inside AppRoutes.Example of usage:
Title can also be provided as a function: ((match: UIMatch) => string)
Example of usage:
I didn't use it , because ADVISORY_ID looks scary IMHO , but if we want to use it in path - we could.
Also worth mentioning that this title changes are client only and if we ll use
curl http://localhost:3000/searchwe would still have default title fromindex.htmltemplate file. In the browser it is dynamically changed by browser's JS.i'm not sure if we could change it to be server side somehow and i'm not sure if we need it server side
Summary by Sourcery
Update the application to set the browser document title based on the active route using reusable hooks and route metadata.
New Features:
Summary by Sourcery
Set per-page browser titles using a shared document metadata component across major application routes.
New Features:
Enhancements: