[OSDEV-2375] Create location name, OS ID, and "Understanding Data Sources" sections#893
Merged
VadimKovalenkoSNF merged 38 commits intomainfrom Mar 3, 2026
Conversation
…mode. Use uuid instead of Math.random()
…s-and-CTA-support
…s-and-CTA-support
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds frontend typography documentation; introduces a new interactive Data Sources UI (toggle, tooltip, item components and frozen constants); refactors LocationTitle to render dynamic name and OS ID with copy actions; adds HandshakeIcon; migrates several styles to theme-derived typography/spacing; adjusts tooltip prop and small typography/colour updates; adds tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant DSInfo as DataSourcesInfo
participant DSItem as DataSourceItem
participant Tooltip as DialogTooltip
User->>DSInfo: Click toggle (show extra info)
DSInfo->>DSInfo: setState(showSubsectionInfo = true)
DSInfo->>DSItem: Render items with showSubsectionInfo=true
User->>DSInfo: Click info IconButton
DSInfo->>Tooltip: Open tooltip (display learn more link)
User->>DSItem: Click "Learn more" link
DSItem->>User: Navigate to item's learnMoreUrl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
doc/frontend.md (1)
106-106: Polish repeated wording in hierarchy guidance.Line 106 repeats “right” close together; consider replacing one instance (e.g., “appropriate”) to improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/frontend.md` at line 106, Edit the sentence that repeats "right" in the hierarchy guidance so it reads more smoothly; in the line starting "Keep **one** clear hierarchy per page: one h1, then h2/h3 as needed; use the table in §2 to pick the right style key and component/variant." replace the second "right" with a synonym like "appropriate" (or "correct"/"suitable") so it becomes "...use the table in §2 to pick the appropriate style key and component/variant." to remove the repeated wording.src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx (1)
77-77: Prevent potential horizontal scroll from the spaced Grid container.
Gridcontainers withspacingcan create negative-margin overflow in this codebase. Add the standard width/margin guard.Proposed fix
- <Grid container className={classes.descriptionList} spacing={2}> + <Grid + container + className={classes.descriptionList} + spacing={2} + style={{ margin: 0, width: '100%' }} + >Based on learnings: In React components that use Material-UI Grid, when a Grid container with spacing causes horizontal scroll, apply
style={{ margin: 0, width: '100%' }}to the container.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx` at line 77, The Grid container in DataSourcesInfo.jsx (the <Grid container ... className={classes.descriptionList} spacing={2} />) can create negative-margin overflow; update that Grid element to include the guard style prop (style={{ margin: 0, width: '100%' }}) so the container no longer causes horizontal scroll while preserving spacing. Ensure you modify the Grid instance that uses classes.descriptionList in the DataSourcesInfo component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx`:
- Line 16: The Grid item in DataSourceItem.jsx (the <Grid item sm={12} md={4}
className={classes.descriptionItem}> element) is missing an explicit xs
breakpoint; update this Grid element to include xs={12} so it mirrors similar
components (e.g., ClaimSubtitleRow/ClaimStatusRow) and ensures full-width
behavior on extra-small screens while preserving sm and md settings.
In
`@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.js`:
- Around line 45-50: The nth-child padding in the descriptionList style is
applied at all sizes and causes misalignment on stacked/small screens; move the
'& > *:nth-child(2)' rule so it only applies at desktop breakpoints (e.g., nest
it inside the theme breakpoint/media query like theme.breakpoints.up('md') or an
equivalent '@media (min-width: ...)' block) so the paddingLeft/paddingRight are
added only on larger screens while leaving small-screen stacking unaffected.
---
Nitpick comments:
In `@doc/frontend.md`:
- Line 106: Edit the sentence that repeats "right" in the hierarchy guidance so
it reads more smoothly; in the line starting "Keep **one** clear hierarchy per
page: one h1, then h2/h3 as needed; use the table in §2 to pick the right style
key and component/variant." replace the second "right" with a synonym like
"appropriate" (or "correct"/"suitable") so it becomes "...use the table in §2 to
pick the appropriate style key and component/variant." to remove the repeated
wording.
In
`@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx`:
- Line 77: The Grid container in DataSourcesInfo.jsx (the <Grid container ...
className={classes.descriptionList} spacing={2} />) can create negative-margin
overflow; update that Grid element to include the guard style prop (style={{
margin: 0, width: '100%' }}) so the container no longer causes horizontal scroll
while preserving spacing. Ensure you modify the Grid instance that uses
classes.descriptionList in the DataSourcesInfo component.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
doc/frontend.mdsrc/react/src/components/ProductionLocation/Heading/ClaimFlag/ClaimFlag.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/constants.jssrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.jssrc/react/src/components/ProductionLocation/Heading/LocationTitle/LocationTitle.jsxsrc/react/src/components/ProductionLocation/Heading/LocationTitle/styles.jssrc/react/src/components/ProductionLocation/ProductionLocationDetailsContent/ProductionLocationDetailsContent.jsx
src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx
Outdated
Show resolved
Hide resolved
src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx (1)
16-16:⚠️ Potential issue | 🟡 MinorAdd explicit
xs={12}for consistent mobile layout (still pending).Line 16 still omits
xs, so extra-small breakpoints can fall back to auto item sizing instead of guaranteed full-width cards.🐛 Proposed fix
- <Grid item sm={12} md={4} className={classes.descriptionItem}> + <Grid item xs={12} sm={12} md={4} className={classes.descriptionItem}>Based on learnings: For Material-UI Grid usage in this repository, breakpoint props like
xsshould be applied on Grid items to maintain predictable responsive behavior.#!/bin/bash # Verify current breakpoint configuration on the DataSourceItem Grid item. cat -n src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx | sed -n '12,22p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx` at line 16, The Grid item in DataSourceItem.jsx currently uses sm={12} md={4} but omits xs, causing unpredictable extra-small layouts; update the Grid element (the one with className={classes.descriptionItem} inside the DataSourceItem component) to include xs={12} so cards are full-width on mobile (i.e., change the Grid props from sm={12} md={4} to xs={12} sm={12} md={4}); no other behavior changes required.
🧹 Nitpick comments (2)
src/react/src/components/HandshakeIcon.jsx (1)
4-49: Consider adding PropTypes for documentation.The component works correctly, but adding PropTypes would improve documentation and provide runtime validation consistent with other components in the codebase.
📝 Suggested PropTypes addition
import React from 'react'; +import PropTypes from 'prop-types'; import SvgIcon from '@material-ui/core/SvgIcon'; export default function HandshakeIcon(props) { return ( <SvgIcon viewBox="0 0 24 24" {...props}> {/* ... paths ... */} </SvgIcon> ); } + +HandshakeIcon.propTypes = { + className: PropTypes.string, + color: PropTypes.string, + fontSize: PropTypes.oneOf(['inherit', 'default', 'small', 'large']), +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/HandshakeIcon.jsx` around lines 4 - 49, Add PropTypes to the HandshakeIcon component: import PropTypes from 'prop-types', then declare HandshakeIcon.propTypes to document/validate incoming props (e.g., accept className, style, sx, color, fontSize or a generic object/any for spread props) and optionally HandshakeIcon.defaultProps for sensible defaults; reference the HandshakeIcon function and its props spread ({...props}) and ensure the prop names match how other icon components define PropTypes in the codebase.src/react/src/util/typographyStyles.js (1)
40-40: Keep shared typography token units consistent with repository convention.Line 40, Line 44, and Line 48 now use
1rem; consider keeping these at16pxto avoid mixed units in core style tokens.♻️ Proposed adjustment
sectionDescription: Object.freeze({ - fontSize: '1rem', // 16px + fontSize: '16px', marginBottom: '10px', }), bodyText: Object.freeze({ - fontSize: '1rem', // 16px + fontSize: '16px', color: theme.palette.text.secondary, }), inlineHighlight: Object.freeze({ - fontSize: '1rem', // 16px + fontSize: '16px', fontWeight: 500, color: theme.palette.text.primary, display: 'inline', }),Based on learnings: In the Open Supply Hub project, when defining font sizes in styles, prefer using
pxunits directly.Also applies to: 44-44, 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/util/typographyStyles.js` at line 40, Shared typography core tokens use '1rem' causing mixed units; change those fontSize entries to '16px' so tokens stay in px units. Locate the fontSize properties (e.g., the token object that currently has fontSize: '1rem') in typographyStyles.js and replace '1rem' with '16px' for all core/shared tokens (the occurrences at the same token keys around the shown snippet and the other instances noted) to keep unit convention consistent across the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx`:
- Line 77: The Grid container rendering (Grid with spacing={2}) can cause
horizontal overflow because the descriptionList class only sets marginTop;
update the CSS rules for the descriptionList style (used where Grid container
className={classes.descriptionList}) to include margin: 0 and width: 100% so the
negative margins from spacing are contained and horizontal overflow is
prevented; modify the descriptionList style definition (where it's declared for
the DataSourcesInfo component) to add these properties.
In
`@src/react/src/components/ProductionLocation/Heading/LocationTitle/LocationTitle.jsx`:
- Line 32: The OS ID label/tooltip block (rendered using classes.osIdRow inside
the LocationTitle component) is shown even when the osId value is empty; update
the JSX to conditionally render that entire OS ID block only when osId is a
non-empty string (e.g., check Boolean(osId) or osId.trim() !== ''), and apply
the same guard to the duplicate OS ID rendering region around the other block
mentioned (the section covering the other OS ID lines ~68-108) so the "OS ID:"
label and tooltip never appear without a value.
---
Duplicate comments:
In
`@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx`:
- Line 16: The Grid item in DataSourceItem.jsx currently uses sm={12} md={4} but
omits xs, causing unpredictable extra-small layouts; update the Grid element
(the one with className={classes.descriptionItem} inside the DataSourceItem
component) to include xs={12} so cards are full-width on mobile (i.e., change
the Grid props from sm={12} md={4} to xs={12} sm={12} md={4}); no other behavior
changes required.
---
Nitpick comments:
In `@src/react/src/components/HandshakeIcon.jsx`:
- Around line 4-49: Add PropTypes to the HandshakeIcon component: import
PropTypes from 'prop-types', then declare HandshakeIcon.propTypes to
document/validate incoming props (e.g., accept className, style, sx, color,
fontSize or a generic object/any for spread props) and optionally
HandshakeIcon.defaultProps for sensible defaults; reference the HandshakeIcon
function and its props spread ({...props}) and ensure the prop names match how
other icon components define PropTypes in the codebase.
In `@src/react/src/util/typographyStyles.js`:
- Line 40: Shared typography core tokens use '1rem' causing mixed units; change
those fontSize entries to '16px' so tokens stay in px units. Locate the fontSize
properties (e.g., the token object that currently has fontSize: '1rem') in
typographyStyles.js and replace '1rem' with '16px' for all core/shared tokens
(the occurrences at the same token keys around the shown snippet and the other
instances noted) to keep unit convention consistent across the file.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/react/src/components/HandshakeIcon.jsxsrc/react/src/components/ProductionLocation/Heading/ClosureStatus/styles.jssrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/constants.jssrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.jssrc/react/src/components/ProductionLocation/Heading/LocationTitle/LocationTitle.jsxsrc/react/src/components/ProductionLocation/Heading/LocationTitle/styles.jssrc/react/src/util/typographyStyles.js
src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx
Show resolved
Hide resolved
src/react/src/components/ProductionLocation/Heading/LocationTitle/LocationTitle.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx (1)
77-77:⚠️ Potential issue | 🟡 MinorAdd an overflow guard to the spacing Grid container.
Line 77 uses
Grid containerwithspacing={2}; without amargin: 0/width: '100%'guard, this can introduce horizontal overflow due to spacing negative margins.#!/bin/bash set -euo pipefail # Verify current container usage sed -n '72,86p' src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx # Verify whether the descriptionList style already contains overflow guards fd -p 'styles.js' src/react/src/components/ProductionLocation/Heading/DataSourcesInfo \ -x sh -c 'echo "==> $1"; rg -n "descriptionList|margin|width" "$1"' sh {}Proposed fix
- <Grid container className={classes.descriptionList} spacing={2}> + <Grid + container + className={classes.descriptionList} + spacing={2} + style={{ margin: 0, width: '100%' }} + >Based on learnings: In React components that use Material-UI Grid, when a Grid container with spacing causes horizontal scroll, apply
style={{ margin: 0, width: '100%' }}to the container.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx` at line 77, The Grid container in DataSourcesInfo.jsx (the element using className={classes.descriptionList} and spacing={2}) can cause horizontal overflow due to spacing negative margins; update the overflow guard by adding margin: 0 and width: '100%' to the container – either inline on the Grid element or by adding these rules to the descriptionList style in the component's styles.js so the Grid container (descriptionList) explicitly sets margin: 0 and width: '100%'.
🧹 Nitpick comments (3)
src/react/src/__tests__/components/LocationTitle.test.jsx (1)
34-39: Optional: align null-data expectations with real runtime contract.Line 34–39 asserts an
OS ID:heading fordata: null; if that state is not reachable in production flow, this test may be brittle and can be replaced with a more representative scenario.Based on learnings: The LocationTitle component will never render with an empty OS ID because this is enforced higher in the hierarchy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/__tests__/components/LocationTitle.test.jsx` around lines 34 - 39, The test "renders without crashing when data is null" relies on a runtime state that doesn't occur; update the test in LocationTitle.test.jsx to use a representative prop shape instead of data: null (or remove the OS ID assertion) by calling renderLocationTitle with realistic data including an OS ID, then assert expected headings accordingly; locate the renderLocationTitle helper and the LocationTitle component reference to adjust the test so it matches the real contract (either remove the expect for the OS ID when passing null, or supply an object with osId and assert its presence).src/react/src/components/ProductionLocation/Heading/LocationTitle/LocationTitle.jsx (1)
48-54: Consider moving tooltip/link/icon inline styles into JSS classes.This would keep styling centralized and easier to theme consistently.
Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Heading/LocationTitle/LocationTitle.jsx` around lines 48 - 54, In LocationTitle.jsx (component LocationTitle) replace the inline styles on the <p> wrapper (marginTop: 8, marginBottom: 0) and the <a> tag (style: { color: 'white' }) — and the similar inline style used at the other occurrence — with JSS class names: add rules (e.g., locationTitleText, learnMoreLink) to the component's JSS/styles object and apply those className props to the <p> and <a> elements; keep the href using OS_ID_LEARN_MORE_URL and retain target/rel, but centralize color/margins in the new JSS classes so the look can be themed consistently.src/react/src/__tests__/components/DataSourcesInfo.test.jsx (1)
23-37: Consolidate the two title-render tests.Line 23–29 and Line 31–37 assert nearly the same heading behavior; merging them would reduce test maintenance noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/__tests__/components/DataSourcesInfo.test.jsx` around lines 23 - 37, Merge the two redundant tests into a single test that calls renderDataSourcesInfo() once and asserts the heading appears with the expected text and level; update or replace the two tests named 'renders without crashing' and 'renders section title "Understanding Data Sources"' with one test (e.g., 'renders "Understanding Data Sources" heading') that uses screen.getByRole('heading', { level: 3, name: 'Understanding Data Sources' }) (or asserts both getByRole('heading', { name: 'Understanding Data Sources' }) and checks its tagName/level) to keep the same assertions while reducing duplication; ensure only renderDataSourcesInfo() is invoked once in the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/frontend.md`:
- Around line 116-117: The Quick Reference table entries for "Page title (e.g.
facility name)" and "Section title" incorrectly list MUI v3 variants as
variant="h1" and variant="h3"; update those entries to use the correct MUI v3
variants variant="headline" for the Page title and variant="title" for the
Section title so the table matches the MUI v3 Typography guidance referenced
earlier and the rest of the docs/codebase.
---
Duplicate comments:
In
`@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsx`:
- Line 77: The Grid container in DataSourcesInfo.jsx (the element using
className={classes.descriptionList} and spacing={2}) can cause horizontal
overflow due to spacing negative margins; update the overflow guard by adding
margin: 0 and width: '100%' to the container – either inline on the Grid element
or by adding these rules to the descriptionList style in the component's
styles.js so the Grid container (descriptionList) explicitly sets margin: 0 and
width: '100%'.
---
Nitpick comments:
In `@src/react/src/__tests__/components/DataSourcesInfo.test.jsx`:
- Around line 23-37: Merge the two redundant tests into a single test that calls
renderDataSourcesInfo() once and asserts the heading appears with the expected
text and level; update or replace the two tests named 'renders without crashing'
and 'renders section title "Understanding Data Sources"' with one test (e.g.,
'renders "Understanding Data Sources" heading') that uses
screen.getByRole('heading', { level: 3, name: 'Understanding Data Sources' })
(or asserts both getByRole('heading', { name: 'Understanding Data Sources' })
and checks its tagName/level) to keep the same assertions while reducing
duplication; ensure only renderDataSourcesInfo() is invoked once in the new
test.
In `@src/react/src/__tests__/components/LocationTitle.test.jsx`:
- Around line 34-39: The test "renders without crashing when data is null"
relies on a runtime state that doesn't occur; update the test in
LocationTitle.test.jsx to use a representative prop shape instead of data: null
(or remove the OS ID assertion) by calling renderLocationTitle with realistic
data including an OS ID, then assert expected headings accordingly; locate the
renderLocationTitle helper and the LocationTitle component reference to adjust
the test so it matches the real contract (either remove the expect for the OS ID
when passing null, or supply an object with osId and assert its presence).
In
`@src/react/src/components/ProductionLocation/Heading/LocationTitle/LocationTitle.jsx`:
- Around line 48-54: In LocationTitle.jsx (component LocationTitle) replace the
inline styles on the <p> wrapper (marginTop: 8, marginBottom: 0) and the <a> tag
(style: { color: 'white' }) — and the similar inline style used at the other
occurrence — with JSS class names: add rules (e.g., locationTitleText,
learnMoreLink) to the component's JSS/styles object and apply those className
props to the <p> and <a> elements; keep the href using OS_ID_LEARN_MORE_URL and
retain target/rel, but centralize color/margins in the new JSS classes so the
look can be themed consistently.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
doc/frontend.mdsrc/react/src/__tests__/components/DataSourcesInfo.test.jsxsrc/react/src/__tests__/components/LocationTitle.test.jsxsrc/react/src/components/Contribute/DialogTooltip.jsxsrc/react/src/components/ProductionLocation/Heading/ClosureStatus/ClosureStatus.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsxsrc/react/src/components/ProductionLocation/Heading/LocationTitle/LocationTitle.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetails/ProductionLocationDetails.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsContainer/ProductionLocationDetailsContainer.jsx
💤 Files with no reviewable changes (1)
- src/react/src/components/Contribute/DialogTooltip.jsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
doc/frontend.md (1)
108-108: Minor wording polish for clarity on Line 108.You use “right” twice in nearby guidance; consider replacing one with “appropriate” to improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/frontend.md` at line 108, The sentence "Keep **one** clear hierarchy per page: one h1, then h2/h3 as needed; use the table in §2 to pick the right style key and component/variant." repeats the word "right"; change one occurrence (e.g., "pick the right style key") to "pick the appropriate style key" so it reads smoothly while preserving meaning and emphasis on a single hierarchy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@doc/frontend.md`:
- Line 108: The sentence "Keep **one** clear hierarchy per page: one h1, then
h2/h3 as needed; use the table in §2 to pick the right style key and
component/variant." repeats the word "right"; change one occurrence (e.g., "pick
the right style key") to "pick the appropriate style key" so it reads smoothly
while preserving meaning and emphasis on a single hierarchy.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
doc/frontend.mdsrc/react/src/__tests__/components/DataSourcesInfo.test.jsxsrc/react/src/__tests__/components/LocationTitle.test.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsxsrc/react/src/components/ProductionLocation/Heading/LocationTitle/styles.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/react/src/components/ProductionLocation/Heading/LocationTitle/styles.js
- src/react/src/tests/components/LocationTitle.test.jsx
- src/react/src/tests/components/DataSourcesInfo.test.jsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.js (1)
44-46:⚠️ Potential issue | 🟡 MinorAdd overflow guards to
descriptionListused by the spacing Grid container.Line 45 only sets
marginTop; this class is applied to aGrid containerwithspacing={2}(inDataSourcesInfo.jsxLine 77), so negative spacing margins can still trigger horizontal scroll.Proposed fix
descriptionList: Object.freeze({ marginTop: 0, + margin: 0, + width: '100%', '& > *:nth-child(2)': { [theme.breakpoints.up('md')]: { paddingLeft: spacing * 2, paddingRight: spacing * 2, }, }, }),Based on learnings: in this repository’s Material-UI Grid usage, spacing-related overflow should be guarded with
margin: 0andwidth: '100%'on the container style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.js` around lines 44 - 46, The descriptionList style applied to the Grid container (used in DataSourcesInfo.jsx on the Grid with spacing={2}) lacks overflow guards; update the descriptionList Object.freeze entry to include margin: 0 and width: '100%' (in addition to marginTop: 0) so the spacing negative margins cannot cause horizontal scroll.
🧹 Nitpick comments (1)
src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx (1)
40-47: Add contextualaria-labelto repeated “Learn more” links.On Line 46, each card exposes identical link text; adding item-specific labeling improves screen-reader link navigation.
Proposed fix
<a href={learnMoreUrl} target="_blank" rel="noopener noreferrer" className={classes.learnMoreLink} + aria-label={`Learn more about ${title}`} > Learn more → </a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx` around lines 40 - 47, The repeated "Learn more →" links in the DataSourceItem component lack contextual aria-labels for screen readers; update the anchor (the element using className={classes.learnMoreLink} and href={learnMoreUrl}) to include an item-specific aria-label such as "Learn more about {dataSourceName}" using the component's title/name prop (e.g., props.name, props.title, or dataSource.name) so each link is uniquely announced to assistive technologies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.js`:
- Around line 44-46: The descriptionList style applied to the Grid container
(used in DataSourcesInfo.jsx on the Grid with spacing={2}) lacks overflow
guards; update the descriptionList Object.freeze entry to include margin: 0 and
width: '100%' (in addition to marginTop: 0) so the spacing negative margins
cannot cause horizontal scroll.
---
Nitpick comments:
In
`@src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsx`:
- Around line 40-47: The repeated "Learn more →" links in the DataSourceItem
component lack contextual aria-labels for screen readers; update the anchor (the
element using className={classes.learnMoreLink} and href={learnMoreUrl}) to
include an item-specific aria-label such as "Learn more about {dataSourceName}"
using the component's title/name prop (e.g., props.name, props.title, or
dataSource.name) so each link is uniquely announced to assistive technologies.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourceItem.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/DataSourcesInfo.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/constants.jssrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.jssrc/react/src/util/COLOURS.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/components/ProductionLocation/Heading/DataSourcesInfo/constants.js
|
vlad-shapik
approved these changes
Mar 3, 2026
This was referenced Mar 5, 2026
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
3 participants
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.
Implements OSDEV-2375
Create location name, OS ID, and "Understanding Data Sources" sections