OBPIH-6272 Add source code with hyperlink to source save success message#5673
Conversation
alannadolny
requested review from
SebastianLib,
awalkowiak,
Copilot,
ewaterman and
kchelstowski
github-actions
Bot
added
type: feature
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5673 +/- ##
============================================
+ Coverage 9.12% 9.70% +0.58%
- Complexity 1170 1322 +152
============================================
Files 701 717 +16
Lines 45281 45803 +522
Branches 10851 10930 +79
============================================
+ Hits 4131 4445 +314
- Misses 40497 40670 +173
- Partials 653 688 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a clickable hyperlink to the product supplier code in success messages when creating or updating a product source. It introduces a new custom hook useTranslateWithRedirect that enables embedding links within translated text by replacing specified phrases with HTML anchor tags. The product supplier form is updated to use this new hook, and the i18n messages are modified to include the product code as a dynamic placeholder.
Key Changes
- Created
useTranslateWithRedirecthook to support translations with embedded hyperlinks - Updated product supplier form success messages to include clickable product code links
- Modified i18n message templates to interpolate the product code
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/js/hooks/useTranslateWithRedirect.jsx |
New hook that extends translation functionality with link replacement capabilities using dangerouslySetInnerHTML |
src/js/hooks/productSupplier/form/useProductSupplierForm.js |
Updated to use useTranslateWithRedirect for success messages, extracting product code from API response and creating clickable links |
grails-app/i18n/messages.properties |
Updated success message templates to include ${code} placeholder for product code interpolation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { | ||
| const anchorTag = `<a href="${redirectTo}" target="_blank">${phrase}</a>`; | ||
| return acc.replace(phrase, anchorTag); |
There was a problem hiding this comment.
The String.replace() method only replaces the first occurrence of the phrase. If the same phrase appears multiple times in the translated text, only the first occurrence will be replaced with a link, leaving subsequent occurrences as plain text. This can lead to inconsistent behavior and confusing user experience.
Use String.replaceAll() instead, or use a regular expression with the global flag to replace all occurrences.
| return acc.replace(phrase, anchorTag); | |
| return acc.replaceAll(phrase, anchorTag); |
There was a problem hiding this comment.
No, it can lead to too many changes in the string. If someone wants to replace more than one occurrence, they can do it by passing that argument more than once. Plus, sometimes the same phase can have different redirects, like the string "Read more."
| const successMessage = translateWithRedirect({ | ||
| label: `react.productSupplier.form.success.${productSupplierId ? 'update' : 'create'}`, | ||
| defaultLabel: `Product ${productSupplierCode} has been ${productSupplierId ? 'updated' : 'created'} successfully`, | ||
| options: { code: productSupplierCode }, | ||
| redirects: [{ | ||
| phrase: productSupplierCode, | ||
| redirectTo: PRODUCT_SUPPLIER_URL.edit(productSupplier), | ||
| }], | ||
| }); |
There was a problem hiding this comment.
The productSupplierCode may be undefined if the API response structure is unexpected or if the code field is missing. Using an undefined value in the phrase parameter will cause the error check on line 43 to throw an error with "The phrase 'undefined' was not found in the translation", and even if it somehow passes, it will create a link with literal text "undefined" which provides a poor user experience.
Add a null/undefined check for productSupplierCode before using it, and provide a fallback behavior when the code is not available.
| }); | ||
|
|
||
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { | ||
| const anchorTag = `<a href="${redirectTo}" target="_blank">${phrase}</a>`; |
There was a problem hiding this comment.
The anchor tag is missing the rel="noopener noreferrer" attribute which is a security best practice when using target="_blank". Without these attributes, the opened page can access the window.opener property and potentially redirect the parent page to a malicious URL (tabnabbing attack).
Add rel="noopener noreferrer" to the anchor tag for security.
| const anchorTag = `<a href="${redirectTo}" target="_blank">${phrase}</a>`; | |
| const anchorTag = `<a href="${redirectTo}" target="_blank" rel="noopener noreferrer">${phrase}</a>`; |
| redirects.forEach(({ phrase }) => { | ||
| if (!content.includes(phrase)) { | ||
| throw new Error(`The phrase "${phrase}" was not found in the translation`); | ||
| } | ||
| }); | ||
|
|
||
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { |
There was a problem hiding this comment.
Throwing an error during render is a poor user experience and will crash the component. If a phrase is not found in the translation (perhaps due to a translation update or configuration error), the entire UI will break with a white screen.
Consider handling this case more gracefully by either logging a warning and returning the original translated text without the link, or rendering an error message to the user while keeping the UI functional.
| redirects.forEach(({ phrase }) => { | |
| if (!content.includes(phrase)) { | |
| throw new Error(`The phrase "${phrase}" was not found in the translation`); | |
| } | |
| }); | |
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { | |
| // For each redirect, check if the phrase exists; warn if not, but do not throw. | |
| redirects.forEach(({ phrase }) => { | |
| if (!content.includes(phrase)) { | |
| console.warn(`useTranslateWithRedirect: The phrase "${phrase}" was not found in the translation for label "${label}". Skipping link replacement for this phrase.`); | |
| } | |
| }); | |
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { | |
| if (!acc.includes(phrase)) { | |
| // If the phrase is not found, skip replacement for this phrase. | |
| return acc; | |
| } |
There was a problem hiding this comment.
It should crash the component, because it means that the developer misuses that hook.
| } | ||
| }); | ||
|
|
||
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { |
There was a problem hiding this comment.
If multiple redirect configurations have overlapping phrases or one phrase is a substring of another, the replacement order may cause unexpected results. For example, if one phrase is "Product ABC" and another is "ABC", the first replacement could inadvertently prevent the second from matching, or create malformed HTML.
Consider validating that phrases don't overlap or contain each other, or process longer phrases first to avoid this issue.
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { | |
| // Sort redirects by phrase length descending to avoid substring replacement issues | |
| const sortedRedirects = [...redirects].sort((a, b) => b.phrase.length - a.phrase.length); | |
| const stringifiedHTML = sortedRedirects.reduce((acc, { phrase, redirectTo }) => { |
There was a problem hiding this comment.
stop overthinking
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { | ||
| const anchorTag = `<a href="${redirectTo}" target="_blank">${phrase}</a>`; | ||
| return acc.replace(phrase, anchorTag); | ||
| }, content); | ||
|
|
||
| return ( | ||
| <span dangerouslySetInnerHTML={{ __html: stringifiedHTML }} /> |
There was a problem hiding this comment.
The use of dangerouslySetInnerHTML with unsanitized user input creates a Cross-Site Scripting (XSS) vulnerability. The phrase and redirectTo parameters are directly interpolated into HTML without any sanitization or escaping. If either of these values contains malicious HTML/JavaScript code, it will be executed in the user's browser.
Consider using a dedicated HTML sanitization library (like DOMPurify) to sanitize the HTML before rendering, or better yet, avoid dangerouslySetInnerHTML entirely by constructing React elements programmatically.
| const stringifiedHTML = redirects.reduce((acc, { phrase, redirectTo }) => { | |
| const anchorTag = `<a href="${redirectTo}" target="_blank">${phrase}</a>`; | |
| return acc.replace(phrase, anchorTag); | |
| }, content); | |
| return ( | |
| <span dangerouslySetInnerHTML={{ __html: stringifiedHTML }} /> | |
| // Replace each phrase with a React <a> element, preserving order and avoiding XSS | |
| let nodes = [content]; | |
| redirects.forEach(({ phrase, redirectTo }, idx) => { | |
| const newNodes = []; | |
| nodes.forEach((node, nodeIdx) => { | |
| if (typeof node === 'string') { | |
| const parts = node.split(phrase); | |
| for (let i = 0; i < parts.length; i++) { | |
| if (parts[i]) newNodes.push(parts[i]); | |
| if (i < parts.length - 1) { | |
| newNodes.push( | |
| <a | |
| key={`redirect-link-${idx}-${nodeIdx}-${i}`} | |
| href={redirectTo} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > | |
| {phrase} | |
| </a> | |
| ); | |
| } | |
| } | |
| } else { | |
| newNodes.push(node); | |
| } | |
| }); | |
| nodes = newNodes; | |
| }); | |
| return ( | |
| <span> | |
| {nodes} | |
| </span> |
There was a problem hiding this comment.
yeah this suggested code is overly complex.
So I suppose the reason you use dangerouslySetInnerHTML is because
- this isn't user input so we don't need to check for XSS
- it's simpler code
you could build it somewhat similarly to copilot such that it doesn't require using dangerouslySetInnerHTML (which has a scary name) but I don't know then frontend well enough to know the benefits so I'll defer to you.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.