[OSDEV-2353] Production location skeleton page#884
Merged
VadimKovalenkoSNF merged 40 commits intomainfrom Feb 24, 2026
Merged
Conversation
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
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/django/api/mail.py (1)
55-61: LGTM — the fix correctly resolves the broken claim URL path.The previous delegation to
make_facility_urlcould produce/production-locations/{id}/claim(no frontend route) whenPRODUCTION_LOCATION_PAGE_SWITCHwas active. Explicitly constructing/facilities/{id}/claimhere is the right fix, and the inline comment communicates the intent clearly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/django/api/mail.py` around lines 55 - 61, Previously the code used make_facility_url which could generate /production-locations/{id}/claim under PRODUCTION_LOCATION_PAGE_SWITCH; replace that delegation with an explicit string construction '/facilities/{id}/claim' using make_oshub_url(request) and location.id (as in the diff) so the old claim flow always returns the correct frontend route, and keep the inline comment explaining why make_facility_url is not used; verify the return expression is exactly '{}/facilities/{}/claim'.format(make_oshub_url(request), location.id).
🧹 Nitpick comments (8)
src/django/api/mail.py (1)
52-52: Consider extracting'enable_v1_claims_flow'as a module-level constant.
PRODUCTION_LOCATION_PAGE_SWITCHis already a named constant at line 16, but'enable_v1_claims_flow'is used as a bare string literal. Extracting it would keep the two switch names consistent and make typos immediately visible.♻️ Proposed refactor
PRODUCTION_LOCATION_PAGE_SWITCH = 'enable_production_location_page' +V1_CLAIMS_FLOW_SWITCH = 'enable_v1_claims_flow'- if switch_is_active('enable_v1_claims_flow'): + if switch_is_active(V1_CLAIMS_FLOW_SWITCH):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/django/api/mail.py` at line 52, Extract the literal 'enable_v1_claims_flow' into a module-level constant (e.g., ENABLE_V1_CLAIMS_FLOW_SWITCH) and replace the bare string in the switch_is_active call with that constant; update the top of the module alongside PRODUCTION_LOCATION_PAGE_SWITCH, and ensure any other occurrences use the new constant to keep switch names consistent and catch typos.src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsx (3)
4-9:Object.freeze()is unnecessary on MUI style objects.MUI v3/JSS builds its own internal representation from the styles thunk and does not mutate the returned object, so
Object.freezeadds noise without benefit. Freezing style objects is a non-standard pattern in this codebase and can be confusing.♻️ Suggested simplification
-const locationStyles = () => - Object.freeze({ - container: Object.freeze({ - backgroundColor: 'grey', - }), - }); +const locationStyles = { + container: { + backgroundColor: 'grey', + }, +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsx` around lines 4 - 9, The style thunk locationStyles unnecessarily wraps its returned objects with Object.freeze (including the nested container object); remove the Object.freeze calls so locationStyles simply returns a plain object with container: { backgroundColor: 'grey' } — update the locationStyles function (and any similar style thunks if present) to stop freezing the style object while keeping the same property names (container) so MUI/JSS can consume it normally.
13-13: TODO: implementFacilityDetailsItemcontent.This is expected for a skeleton PR. Let me know if you'd like me to scaffold the
FacilityDetailsItemintegration or open a tracking issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsx` at line 13, The TODO indicates the ProductionLocationDetailsLocation component should render facility details via the FacilityDetailsItem component; replace the placeholder comment in ProductionLocationDetailsLocation.jsx with a proper import of FacilityDetailsItem and pass the required props (e.g., facility object/id, label, and any onEdit or onClick handlers expected by FacilityDetailsItem). Ensure you locate the ProductionLocationDetailsLocation functional component, add "import FacilityDetailsItem from '...'" at the top, and render <FacilityDetailsItem .../> where the TODO comment is, mapping local props/state (such as productionLocation or facilityId) to the FacilityDetailsItem API and forwarding any callbacks or aria/test ids used in this component.
11-17:classesprop injected bywithStylesis never consumed.The component signature accepts no props, so the style rules defined in
locationStylesare never applied. While harmless for a skeleton, theclassesprop will need to be wired up once the TODO is implemented.♻️ Skeleton with classes wired up, ready for the TODO
-const ProductionLocationDetailsLocation = () => ( +const ProductionLocationDetailsLocation = ({ classes }) => ( <> {/** TODO: refer to FacilityDetailsItem */} </> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsx` around lines 11 - 17, The component ProductionLocationDetailsLocation is wrapped with withStyles(locationStyles) but does not accept or use the injected classes prop; update the component signature to accept props (or destructure { classes }) and apply the classes to the JSX elements you add for the TODO (e.g., pass classes.root, classes.item, etc., to the container and FacilityDetailsItem-like elements) so the style rules from locationStyles are actually used while keeping the export withStyles(locationStyles)(ProductionLocationDetailsLocation).src/react/src/components/ProductionLocation/Heading/ClosureStatus.jsx (1)
133-136: Redundant inlinefontSizestyle duplicates the CSS class value
classes.iconalready setsfontSize: '24px'(line 31). The inlinestyle={{ fontSize: '24px' }}on the<i>element is dead code.♻️ Proposed fix
- <i - className={`${classes.text} ${classes.icon} far fa-fw fa-store-slash`} - style={{ fontSize: '24px' }} - /> + <i + className={`${classes.text} ${classes.icon} far fa-fw fa-store-slash`} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Heading/ClosureStatus.jsx` around lines 133 - 136, Remove the redundant inline style on the icon element in ClosureStatus.jsx: the <i> element currently has style={{ fontSize: '24px' }}, but classes.icon already defines fontSize: '24px'; delete the inline style and rely on classes.icon for sizing to avoid dead code and duplicate styling (look for the <i> with className={`${classes.text} ${classes.icon} far fa-fw fa-store-slash`} in the ClosureStatus component).src/react/src/components/ProductionLocation/Sidebar/NavBar.jsx (2)
23-48: React Router v5<Link to="#hash">does not scroll to anchor targets.
react-router-hash-linkexists precisely as "a solution to React Router's issue of not scrolling to#hash-fragmentswhen using the<Link>component." As-is, clicking any of these eight "Jump to" links will update the URL hash but the page will not scroll. This will also work for elements that are created after an asynchronous data load. Since this is a skeleton page it can be deferred, but the UX will be silently broken until resolved.♻️ Proposed fix using
react-router-hash-link-import { Link } from 'react-router-dom'; +import { HashLink as Link } from 'react-router-hash-link';Note that BrowserRouter is required for this to work; usage matches a standard RRv4/v5
<Link>with thetoprop. Thetoprops on eachMenuItemneed no further changes.Verify that
react-router-hash-linkis already a project dependency before adding the import:#!/bin/bash # Check if react-router-hash-link is already installed cat src/react/package.json | grep "react-router-hash-link"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Sidebar/NavBar.jsx` around lines 23 - 48, The current NavBar uses react-router Link for in-page hashes which updates the URL but doesn't scroll; replace the hash-target links with react-router-hash-link's HashLink (e.g., import HashLink from 'react-router-hash-link' and use HashLink in place of Link for the MenuItem children) so clicks scroll to anchors (optionally enable smooth scrolling via the HashLink API); ensure you update the import in NavBar.jsx (where Link is referenced) and verify react-router-hash-link is present in package.json before adding the import.
23-48: Hash navigation requiresHashLinkfromreact-router-hash-linkto scroll to anchors.The current
<Link to="#overview">updates the URL hash but won't scroll to the matching element. The packagereact-router-hash-linkis already installed in the project, so implementing this is straightforward.♻️ Drop-in replacement
-import { Link } from 'react-router-dom'; +import { HashLink as Link } from 'react-router-hash-link';No template changes needed—
HashLinkhandles the scroll automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Sidebar/NavBar.jsx` around lines 23 - 48, Replace the plain react-router <Link> usages in the NavBar component with HashLink from react-router-hash-link so clicks scroll to anchors; import { HashLink as Link } from 'react-router-hash-link' (or import HashLink and rename) at the top of NavBar.jsx and update each <Link to="#..."> inside MenuList/MenuItem (e.g., the Overview, Location, Claimed data, Assessments and Audits, Certifications, Emissions, Living Wage, Grievance Mechanism links) to use HashLink so the page will perform hash scrolling automatically.src/react/src/components/ProductionLocation/ProductionLocationDetailsContent.jsx (1)
49-49: Optional: addclearFacilityto theuseEffectdependency array.
clearFacilityis used in the cleanup callback but omitted from the deps array, which will trigger thereact-hooks/exhaustive-depslint rule. Since it's a stable dispatch-bound function the omission is harmless in practice, but aligning with the rule avoids a lint warning.♻️ Suggested fix
- useEffect(() => () => clearFacility(), []); + useEffect(() => () => clearFacility(), [clearFacility]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsContent.jsx` at line 49, The useEffect cleanup currently calls clearFacility but omits it from the dependency array; update the effect in ProductionLocationDetailsContent (the useEffect that returns () => clearFacility()) to include clearFacility in the deps array so the hook signature becomes useEffect(() => () => clearFacility(), [clearFacility]) to satisfy react-hooks/exhaustive-deps while keeping behavior intact.
🤖 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/ClosureStatus.jsx`:
- Around line 56-61: The isPending branch in ClosureStatus.jsx calls
report.closure_state.toLowerCase(), which can throw if closure_state is
null/undefined; update the isPending rendering in the ClosureStatus component to
safely access closure_state (e.g., use optional chaining or coerce to a string
and provide a sensible default like 'pending' or an empty string) before calling
toLowerCase so the Typography render never throws when report.closure_state is
missing.
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsContent.jsx`:
- Around line 85-92: Remove the erroneous xs={12} from the Grid container in
ProductionLocationDetailsContent.jsx (the Grid with container prop) and add
xs={12} to each child Grid item (the ones wrapping <GeneralFields /> and
<DetailsMap />) so the items explicitly stack full-width on extra-small screens;
keep their existing sm and md props intact.
- Around line 112-119: The conditional in searchForFacilities is redundant
because FACILITIES_REQUEST_PAGE_SIZE equals 50; update the fetchFacilities call
in ProductionLocationDetailsContent.jsx to remove the ternary and use the single
intended pageSize (50) or replace FACILITIES_REQUEST_PAGE_SIZE with a distinct
constant if you plan a different value for the vector-tile path later; apply the
same cleanup to the identical patterns in FacilityDetailsContent.jsx and
FilterSidebarSearchTab.jsx, targeting the searchForFacilities dispatch and the
fetchFacilities({ pageSize }) argument to eliminate the unnecessary
vectorTilesAreActive ? FACILITIES_REQUEST_PAGE_SIZE : 50 expression.
In `@src/react/src/components/ProductionLocation/Sidebar/NavBar.jsx`:
- Around line 10-20: navBarStyles currently only defines container but NavBar
references classes.title; either add a title key to navBarStyles or remove the
className={classes.title} from the NavBar JSX. To fix, update the navBarStyles
function to include a title property (e.g., title: Object.freeze({...}) with the
desired typography/margin styles) so classes.title is defined, or delete the
className usage in the NavBar component to avoid referencing an undefined style.
- Around line 10-20: The NavBar component references classes.title but
navBarStyles only defines container, so classes.title is undefined; fix by
either adding a title style key to navBarStyles (e.g., extend navBarStyles to
include a frozen title object with the desired CSS properties) or remove the
className={classes.title} from the Typography in NavBar if no custom styling is
required; update the navBarStyles and/or NavBar JSX accordingly to keep keys
consistent.
---
Duplicate comments:
In `@src/django/api/mail.py`:
- Around line 55-61: Previously the code used make_facility_url which could
generate /production-locations/{id}/claim under PRODUCTION_LOCATION_PAGE_SWITCH;
replace that delegation with an explicit string construction
'/facilities/{id}/claim' using make_oshub_url(request) and location.id (as in
the diff) so the old claim flow always returns the correct frontend route, and
keep the inline comment explaining why make_facility_url is not used; verify the
return expression is exactly
'{}/facilities/{}/claim'.format(make_oshub_url(request), location.id).
---
Nitpick comments:
In `@src/django/api/mail.py`:
- Line 52: Extract the literal 'enable_v1_claims_flow' into a module-level
constant (e.g., ENABLE_V1_CLAIMS_FLOW_SWITCH) and replace the bare string in the
switch_is_active call with that constant; update the top of the module alongside
PRODUCTION_LOCATION_PAGE_SWITCH, and ensure any other occurrences use the new
constant to keep switch names consistent and catch typos.
In `@src/react/src/components/ProductionLocation/Heading/ClosureStatus.jsx`:
- Around line 133-136: Remove the redundant inline style on the icon element in
ClosureStatus.jsx: the <i> element currently has style={{ fontSize: '24px' }},
but classes.icon already defines fontSize: '24px'; delete the inline style and
rely on classes.icon for sizing to avoid dead code and duplicate styling (look
for the <i> with className={`${classes.text} ${classes.icon} far fa-fw
fa-store-slash`} in the ClosureStatus component).
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsContent.jsx`:
- Line 49: The useEffect cleanup currently calls clearFacility but omits it from
the dependency array; update the effect in ProductionLocationDetailsContent (the
useEffect that returns () => clearFacility()) to include clearFacility in the
deps array so the hook signature becomes useEffect(() => () => clearFacility(),
[clearFacility]) to satisfy react-hooks/exhaustive-deps while keeping behavior
intact.
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsx`:
- Around line 4-9: The style thunk locationStyles unnecessarily wraps its
returned objects with Object.freeze (including the nested container object);
remove the Object.freeze calls so locationStyles simply returns a plain object
with container: { backgroundColor: 'grey' } — update the locationStyles function
(and any similar style thunks if present) to stop freezing the style object
while keeping the same property names (container) so MUI/JSS can consume it
normally.
- Line 13: The TODO indicates the ProductionLocationDetailsLocation component
should render facility details via the FacilityDetailsItem component; replace
the placeholder comment in ProductionLocationDetailsLocation.jsx with a proper
import of FacilityDetailsItem and pass the required props (e.g., facility
object/id, label, and any onEdit or onClick handlers expected by
FacilityDetailsItem). Ensure you locate the ProductionLocationDetailsLocation
functional component, add "import FacilityDetailsItem from '...'" at the top,
and render <FacilityDetailsItem .../> where the TODO comment is, mapping local
props/state (such as productionLocation or facilityId) to the
FacilityDetailsItem API and forwarding any callbacks or aria/test ids used in
this component.
- Around line 11-17: The component ProductionLocationDetailsLocation is wrapped
with withStyles(locationStyles) but does not accept or use the injected classes
prop; update the component signature to accept props (or destructure { classes
}) and apply the classes to the JSX elements you add for the TODO (e.g., pass
classes.root, classes.item, etc., to the container and FacilityDetailsItem-like
elements) so the style rules from locationStyles are actually used while keeping
the export withStyles(locationStyles)(ProductionLocationDetailsLocation).
In `@src/react/src/components/ProductionLocation/Sidebar/NavBar.jsx`:
- Around line 23-48: The current NavBar uses react-router Link for in-page
hashes which updates the URL but doesn't scroll; replace the hash-target links
with react-router-hash-link's HashLink (e.g., import HashLink from
'react-router-hash-link' and use HashLink in place of Link for the MenuItem
children) so clicks scroll to anchors (optionally enable smooth scrolling via
the HashLink API); ensure you update the import in NavBar.jsx (where Link is
referenced) and verify react-router-hash-link is present in package.json before
adding the import.
- Around line 23-48: Replace the plain react-router <Link> usages in the NavBar
component with HashLink from react-router-hash-link so clicks scroll to anchors;
import { HashLink as Link } from 'react-router-hash-link' (or import HashLink
and rename) at the top of NavBar.jsx and update each <Link to="#..."> inside
MenuList/MenuItem (e.g., the Overview, Location, Claimed data, Assessments and
Audits, Certifications, Emissions, Living Wage, Grievance Mechanism links) to
use HashLink so the page will perform hash scrolling automatically.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/django/api/mail.pysrc/react/src/Routes.jsxsrc/react/src/components/ProductionLocation/Heading/ClosureStatus.jsxsrc/react/src/components/ProductionLocation/PartnerSection/AssessmentsAndAudits.jsxsrc/react/src/components/ProductionLocation/PartnerSection/Certifications.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsContent.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsxsrc/react/src/components/ProductionLocation/Sidebar/ContributeFields.jsxsrc/react/src/components/ProductionLocation/Sidebar/NavBar.jsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/react/src/components/ProductionLocation/Sidebar/ContributeFields.jsx
- src/react/src/Routes.jsx
- src/react/src/components/ProductionLocation/PartnerSection/AssessmentsAndAudits.jsx
src/react/src/components/ProductionLocation/Heading/ClosureStatus.jsx
Outdated
Show resolved
Hide resolved
...nts/ProductionLocation/ProductionLocationDetailsContent/ProductionLocationDetailsContent.jsx
Show resolved
Hide resolved
...nts/ProductionLocation/ProductionLocationDetailsContent/ProductionLocationDetailsContent.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsx (1)
4-13: Use the styled container (or drop the styles) to avoid dead code.
withStylesandlocationStylesare currently unused because the component returns an empty fragment, so the styling/HOC adds no value. Consider rendering a minimal container to make the placeholder visible (or remove the styles entirely).💡 Suggested fix
-const ProductionLocationDetailsLocation = () => ( - <>{/** TODO: refer to FacilityDetailsItem */}</> -); +const ProductionLocationDetailsLocation = ({ classes }) => ( + <div className={classes.container}> + {/* TODO: refer to FacilityDetailsItem */} + </div> +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsx` around lines 4 - 13, The component ProductionLocationDetailsLocation currently returns an empty fragment causing the locationStyles (and any withStyles HOC) to be dead code; either remove locationStyles and any unused HOC imports or render a minimal styled container using the defined locationStyles (e.g., a div/span that uses the container style or class from locationStyles) so the placeholder is visible—update ProductionLocationDetailsLocation to return the styled element (or delete locationStyles and unused withStyles) and ensure references to locationStyles.container and the withStyles HOC are updated/removed accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsx`:
- Around line 4-13: The component ProductionLocationDetailsLocation currently
returns an empty fragment causing the locationStyles (and any withStyles HOC) to
be dead code; either remove locationStyles and any unused HOC imports or render
a minimal styled container using the defined locationStyles (e.g., a div/span
that uses the container style or class from locationStyles) so the placeholder
is visible—update ProductionLocationDetailsLocation to return the styled element
(or delete locationStyles and unused withStyles) and ensure references to
locationStyles.container and the withStyles HOC are updated/removed accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/django/api/mail.pysrc/react/src/components/ProductionLocation/PartnerSection/AssessmentsAndAudits.jsxsrc/react/src/components/ProductionLocation/PartnerSection/Emissions.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsLocation.jsxsrc/react/src/components/ProductionLocation/Sidebar/NavBar.jsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/react/src/components/ProductionLocation/Sidebar/NavBar.jsx
- src/react/src/components/ProductionLocation/PartnerSection/Emissions.jsx
- src/react/src/components/ProductionLocation/PartnerSection/AssessmentsAndAudits.jsx
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/react/src/components/ProductionLocation/ProductionLocationDetailsContainer/index.jsx (1)
17-20: ConfirmContributeFieldsdoes not requireosId.Line 19 still renders
ContributeFieldswithout anosIdprop. If it still builds URLs fromosId, please passmatch.params.osID; otherwise confirm the prop is no longer required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsContainer/index.jsx` around lines 17 - 20, ContributeFields is being rendered without the osId prop—verify whether ContributeFields still builds URLs from osId; if it does, pass the route param by updating the render to provide osId={match.params.osID} (or the correctly-cased param used in routing) so ContributeFields receives the expected identifier; if ContributeFields no longer needs osId, update its PropTypes/TypeScript signature and any internal references (in the ContributeFields component) to remove the requirement and ensure callers (like ProductionLocationDetailsContainer) no longer pass it.src/react/src/components/ProductionLocation/ProductionLocationDetailsContent/index.jsx (1)
70-82: Grid container still hasxsbreakpoint (previously noted).
xsshould be applied to Grid items rather than containers to avoid layout quirks on small screens.Material-UI v3 Grid: are breakpoint props like `xs` valid on `container` grids or only on `item` grids?Based on learnings: Guideline: For Material-UI Grid usage, avoid horizontal scroll caused by Grid containers with spacing. Use xs (and other breakpoint props) on Grid items, not on Grid containers. Ensure containers maintain spacing for layout, but children that represent columns are Grid items with an appropriate xs (and possibly sm/md) prop. This prevents negative margins from spacing prop causing overflow. Apply this rule to all JSX components under src/react/src/components (not just ProductionLocationDetailsContent.jsx).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsContent/index.jsx` around lines 70 - 82, The outer Grid in ProductionLocationDetailsContent is using the breakpoint prop xs on a container which can cause horizontal overflow; remove xs={12} from the container Grid and instead apply xs={12} to the child Grid items that represent columns (add xs={12} to the Grid items rendering GeneralFields and DetailsMap alongside their existing sm/md props). Update any similar usages across other JSX components under the components directory so breakpoint props like xs are only used on Grid items, not on container Grids.
🧹 Nitpick comments (7)
src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/styles.js (1)
1-5: Avoid recreating frozen styles on every call.You can keep the same function signature but return a single frozen object to reduce allocations.
♻️ Suggested change
-export default () => - Object.freeze({ - container: Object.freeze({ - backgroundColor: 'grey', - }), - }); +const styles = Object.freeze({ + container: Object.freeze({ + backgroundColor: 'grey', + }), +}); + +export default () => styles;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/styles.js` around lines 1 - 5, The current default-exported function recreates and freezes a new styles object on every call; change it to retain the same function signature but return a single pre-built frozen object (e.g., build a constant like STYLES once using Object.freeze for the root and container) so subsequent calls simply return that constant; update the exported default function to return this cached STYLES and keep the existing keys such as container and backgroundColor unchanged.src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/index.jsx (2)
7-7: TODO:FacilityDetailsItemintegration is unimplemented.The inline TODO marks pending implementation of the core content for this component.
Would you like me to scaffold the
FacilityDetailsItemintegration stub, or open a new issue to track this task?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/index.jsx` at line 7, ProductionLocationDetailsLocation currently contains only a TODO fragment and doesn't render the required FacilityDetailsItem; replace the placeholder fragment in the ProductionLocationDetailsLocation component with a proper integration: import FacilityDetailsItem, pass the necessary props (e.g., facility data, address, contact props) from the existing component's props/state into FacilityDetailsItem, and ensure any required prop transformations or null checks are applied (e.g., handle missing facility by rendering a fallback or nothing). Locate the ProductionLocationDetailsLocation component and update its return to render FacilityDetailsItem with correctly mapped props and PropTypes/TypeScript types as needed.
6-10:withStylesinjectsclassesbut the component never consumes it.
withStyles(styles)wraps the component and injects aclassesprop, yet the component signature accepts no props. Since there are no styles applied yet, you can defer thewithStyleswrapper until actual styles are needed, or at minimum accept and ignore the prop as a placeholder.♻️ Proposed adjustment
-const ProductionLocationDetailsLocation = () => ( +const ProductionLocationDetailsLocation = ({ classes }) => ( // eslint-disable-line no-unused-vars <>{/** TODO: refer to FacilityDetailsItem */}</> );Or simply remove the HOC until real styles are introduced:
-import { withStyles } from '@material-ui/core/styles'; - -import styles from './styles'; - const ProductionLocationDetailsLocation = () => ( <>{/** TODO: refer to FacilityDetailsItem */}</> ); -export default withStyles(styles)(ProductionLocationDetailsLocation); +export default ProductionLocationDetailsLocation;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/index.jsx` around lines 6 - 10, The component ProductionLocationDetailsLocation is wrapped with withStyles(styles) which injects a classes prop but the function signature accepts no props; either remove the withStyles(styles) wrapper until styles are needed or update ProductionLocationDetailsLocation to accept props (e.g., ({ classes }) ) and ignore or forward classes for future use; locate the component declaration ProductionLocationDetailsLocation and the HOC usage withStyles(styles) and either remove that HOC or change the component to accept and (optionally) pass classes to FacilityDetailsItem or other children so the injected prop is consumed.src/react/src/components/ProductionLocation/ProductionLocationDetails/index.jsx (1)
36-38:filtersandembeddedare mapped from state but never consumed by the component.The component destructures only
{ classes, clearFacility, history: { push } }on line 14, sofiltersandembeddedare unused props. This triggers unnecessary re-renders whenever those state slices change.Proposed fix — remove unused state mapping or use null
-function mapStateToProps({ filters, embeddedMap: { embed } }) { - return { filters, embedded: !!embed }; -} +function mapStateToProps() { + return {}; +}Or simply pass
nullas the first argument toconnect:export default connect( - mapStateToProps, + null, mapDispatchToProps, )(withStyles(styles)(ProductionLocationDetails));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetails/index.jsx` around lines 36 - 38, mapStateToProps currently returns filters and embedded which are never consumed by the ProductionLocationDetails component (only { classes, clearFacility, history: { push } } are destructured), causing unnecessary re-renders; to fix, remove or stop exporting/using mapStateToProps and update the connect call for ProductionLocationDetails to pass null as the first argument (or remove filters/embedded from mapStateToProps entirely) so that filters and embedded are not mapped into props and no longer trigger re-renders.src/react/src/components/ProductionLocation/Sidebar/SupplyChain/styles.js (1)
1-9: Highly duplicated style object across multiple components.This file is byte-for-byte identical to
DataSourcesInfo/styles.js,LocationTitle/styles.js, andClaimDataContainer/styles.js. If these components are expected to share the same base styles long-term, consider extracting a shared style factory (e.g.,components/ProductionLocation/sharedStyles.js) and importing it. If the styles are expected to diverge as the skeleton fills in, the current approach is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Sidebar/SupplyChain/styles.js` around lines 1 - 9, This file exports a frozen style factory that is identical to DataSourcesInfo/styles.js, LocationTitle/styles.js, and ClaimDataContainer/styles.js; extract the common Object.freeze({ container: ..., title: ... }) factory into a shared module (e.g., components/ProductionLocation/sharedStyles.js) and have this file (and the other three) import and re-export or call that shared factory instead of duplicating the same object; update the export in this file to return the shared factory result (preserving the theme parameter) and keep the unique keys container and title so existing consumers (container, title) continue to work.src/react/src/components/ProductionLocation/Heading/ClaimFlag/styles.js (1)
3-39: Optional: deep-freeze nested style objects for consistency.
Other ProductionLocation styles freeze nested objects too; doing the same here avoids accidental mutation.♻️ Suggested update
export default theme => Object.freeze({ - rootClaimed: { + rootClaimed: Object.freeze({ backgroundColor: COLOURS.GREEN, color: '#191919', display: 'flex', justifyContent: 'center', - }, - rootPending: { + }), + rootPending: Object.freeze({ backgroundColor: COLOURS.NAVIGATION, color: '#191919', display: 'flex', justifyContent: 'center', - }, - rootUnclaimed: { + }), + rootUnclaimed: Object.freeze({ backgroundColor: COLOURS.LIGHT_RED, color: '#191919', display: 'flex', justifyContent: 'center', - }, - contentContainer: { + }), + contentContainer: Object.freeze({ width: '100%', maxWidth: '1072px', display: 'flex', alignItems: 'center', flexWrap: 'wrap', paddingRight: theme.spacing.unit, paddingBottom: theme.spacing.unit, - }, - link: { + }), + link: Object.freeze({ color: theme.palette.primary.main, - }, - itemPadding: { + }), + itemPadding: Object.freeze({ paddingLeft: theme.spacing.unit * 3, paddingTop: theme.spacing.unit, paddingBottom: theme.spacing.unit / 4, - }, + }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/Heading/ClaimFlag/styles.js` around lines 3 - 39, The styles export currently calls Object.freeze on the top-level object but leaves nested style objects mutable; update the export (the default theme => Object.freeze({...}) block) to deep-freeze nested properties (rootClaimed, rootPending, rootUnclaimed, contentContainer, link, itemPadding) as well — either by applying a small recursive deepFreeze helper before returning or by individually freezing each nested object after creation so all nested style objects are immutable like the other ProductionLocation styles.src/react/src/components/ProductionLocation/ProductionLocationDetailsMap/styles.js (1)
1-6: Optional: freezecontainerfor consistent immutability.Most style modules in this PR freeze nested objects;
containerremains mutable here. Consider freezing it to keep the immutability pattern consistent.♻️ Suggested tweak
export default theme => Object.freeze({ - container: { + container: Object.freeze({ backgroundColor: 'white', - }, + }), title: Object.freeze({ marginBottom: theme.spacing.unit, }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/ProductionLocation/ProductionLocationDetailsMap/styles.js` around lines 1 - 6, The styles object freezes nested objects but leaves container mutable; freeze the container object to maintain consistent immutability by wrapping the container value with Object.freeze (i.e., freeze the object assigned to the container key in the default exported theme function alongside existing frozen title), so update the exported object to call Object.freeze on the container definition (reference: container and the default export function that currently returns Object.freeze({...})).
🤖 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/ClosureStatus/utils.jsx`:
- Around line 20-24: Guard the render against missing report.closure_state by
ensuring you only call toLowerCase on a string: inside the isPending branch (the
JSX returning <Typography className={classes.text} ...>), compute a safe value
for the closure state (e.g. use optional chaining or a fallback like
report.closure_state || 'unknown' or String(report.closure_state || 'unknown'))
and call toLowerCase on that safe value before interpolating it into the message
so the component never attempts to call toLowerCase on undefined/null.
In
`@src/react/src/components/ProductionLocation/PartnerSection/ParentSectionItem/index.jsx`:
- Around line 22-27: The Switch usage currently has a no-op onChange and
checked={false}, making it look interactive; update the Switch (the component
instance using className={classes.switchWrapper}) to be explicitly
non-interactive by adding the disabled prop and an accessible label via
inputProps (e.g., inputProps={{ 'aria-label': 'read-only partner switch' }}), so
screen readers and users understand it is read-only.
- Around line 2-22: The tooltip icon (InfoIcon) in ParentSectionItem is not
keyboard-accessible; update the DialogTooltip usage so the childComponent is an
IconButton wrapping InfoIcon and add an appropriate aria-label (e.g., "More
information") on the IconButton to make it focusable and screen-reader friendly;
locate the DialogTooltip invocation in ParentSectionItem and replace the bare
InfoIcon with an IconButton (imported from `@material-ui/core/IconButton`) that
contains <InfoIcon /> and includes the aria-label.
In
`@src/react/src/components/ProductionLocation/PartnerSection/PartnerDataContainer/index.jsx`:
- Around line 12-62: Grid items inside the Grid container
(className={classes.root}) only use md={12}, which lets them collapse on small
screens and can cause horizontal scroll; update each Grid item that renders a
ParentSectionItem (and the Typography Grid items) to include xs={12} alongside
md={12} so they occupy full width on small viewports and prevent layout
overflow.
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsContainer/index.jsx`:
- Line 15: In ProductionLocationDetailsContainer update the top-level
Material-UI Grid element (the one with container and className={classes.root})
to remove the xs={12} prop so the container does not get a breakpoint width;
keep spacing={8} on the container and leave the child Grid items (xs={12} md={2}
and xs={12} md={10}) as-is to control layout.
---
Duplicate comments:
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsContainer/index.jsx`:
- Around line 17-20: ContributeFields is being rendered without the osId
prop—verify whether ContributeFields still builds URLs from osId; if it does,
pass the route param by updating the render to provide osId={match.params.osID}
(or the correctly-cased param used in routing) so ContributeFields receives the
expected identifier; if ContributeFields no longer needs osId, update its
PropTypes/TypeScript signature and any internal references (in the
ContributeFields component) to remove the requirement and ensure callers (like
ProductionLocationDetailsContainer) no longer pass it.
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsContent/index.jsx`:
- Around line 70-82: The outer Grid in ProductionLocationDetailsContent is using
the breakpoint prop xs on a container which can cause horizontal overflow;
remove xs={12} from the container Grid and instead apply xs={12} to the child
Grid items that represent columns (add xs={12} to the Grid items rendering
GeneralFields and DetailsMap alongside their existing sm/md props). Update any
similar usages across other JSX components under the components directory so
breakpoint props like xs are only used on Grid items, not on container Grids.
---
Nitpick comments:
In `@src/react/src/components/ProductionLocation/Heading/ClaimFlag/styles.js`:
- Around line 3-39: The styles export currently calls Object.freeze on the
top-level object but leaves nested style objects mutable; update the export (the
default theme => Object.freeze({...}) block) to deep-freeze nested properties
(rootClaimed, rootPending, rootUnclaimed, contentContainer, link, itemPadding)
as well — either by applying a small recursive deepFreeze helper before
returning or by individually freezing each nested object after creation so all
nested style objects are immutable like the other ProductionLocation styles.
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetails/index.jsx`:
- Around line 36-38: mapStateToProps currently returns filters and embedded
which are never consumed by the ProductionLocationDetails component (only {
classes, clearFacility, history: { push } } are destructured), causing
unnecessary re-renders; to fix, remove or stop exporting/using mapStateToProps
and update the connect call for ProductionLocationDetails to pass null as the
first argument (or remove filters/embedded from mapStateToProps entirely) so
that filters and embedded are not mapped into props and no longer trigger
re-renders.
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/index.jsx`:
- Line 7: ProductionLocationDetailsLocation currently contains only a TODO
fragment and doesn't render the required FacilityDetailsItem; replace the
placeholder fragment in the ProductionLocationDetailsLocation component with a
proper integration: import FacilityDetailsItem, pass the necessary props (e.g.,
facility data, address, contact props) from the existing component's props/state
into FacilityDetailsItem, and ensure any required prop transformations or null
checks are applied (e.g., handle missing facility by rendering a fallback or
nothing). Locate the ProductionLocationDetailsLocation component and update its
return to render FacilityDetailsItem with correctly mapped props and
PropTypes/TypeScript types as needed.
- Around line 6-10: The component ProductionLocationDetailsLocation is wrapped
with withStyles(styles) which injects a classes prop but the function signature
accepts no props; either remove the withStyles(styles) wrapper until styles are
needed or update ProductionLocationDetailsLocation to accept props (e.g., ({
classes }) ) and ignore or forward classes for future use; locate the component
declaration ProductionLocationDetailsLocation and the HOC usage
withStyles(styles) and either remove that HOC or change the component to accept
and (optionally) pass classes to FacilityDetailsItem or other children so the
injected prop is consumed.
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/styles.js`:
- Around line 1-5: The current default-exported function recreates and freezes a
new styles object on every call; change it to retain the same function signature
but return a single pre-built frozen object (e.g., build a constant like STYLES
once using Object.freeze for the root and container) so subsequent calls simply
return that constant; update the exported default function to return this cached
STYLES and keep the existing keys such as container and backgroundColor
unchanged.
In
`@src/react/src/components/ProductionLocation/ProductionLocationDetailsMap/styles.js`:
- Around line 1-6: The styles object freezes nested objects but leaves container
mutable; freeze the container object to maintain consistent immutability by
wrapping the container value with Object.freeze (i.e., freeze the object
assigned to the container key in the default exported theme function alongside
existing frozen title), so update the exported object to call Object.freeze on
the container definition (reference: container and the default export function
that currently returns Object.freeze({...})).
In `@src/react/src/components/ProductionLocation/Sidebar/SupplyChain/styles.js`:
- Around line 1-9: This file exports a frozen style factory that is identical to
DataSourcesInfo/styles.js, LocationTitle/styles.js, and
ClaimDataContainer/styles.js; extract the common Object.freeze({ container: ...,
title: ... }) factory into a shared module (e.g.,
components/ProductionLocation/sharedStyles.js) and have this file (and the other
three) import and re-export or call that shared factory instead of duplicating
the same object; update the export in this file to return the shared factory
result (preserving the theme parameter) and keep the unique keys container and
title so existing consumers (container, title) continue to work.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
src/react/src/components/ProductionLocation/ClaimSection/ClaimDataContainer/index.jsxsrc/react/src/components/ProductionLocation/ClaimSection/ClaimDataContainer/styles.jssrc/react/src/components/ProductionLocation/Heading/ClaimFlag/index.jsxsrc/react/src/components/ProductionLocation/Heading/ClaimFlag/styles.jssrc/react/src/components/ProductionLocation/Heading/ClaimFlag/utils.jssrc/react/src/components/ProductionLocation/Heading/ClosureStatus/index.jsxsrc/react/src/components/ProductionLocation/Heading/ClosureStatus/styles.jssrc/react/src/components/ProductionLocation/Heading/ClosureStatus/utils.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/index.jsxsrc/react/src/components/ProductionLocation/Heading/DataSourcesInfo/styles.jssrc/react/src/components/ProductionLocation/Heading/LocationTitle/index.jsxsrc/react/src/components/ProductionLocation/Heading/LocationTitle/styles.jssrc/react/src/components/ProductionLocation/PartnerSection/AssessmentsAndAudits/index.jsxsrc/react/src/components/ProductionLocation/PartnerSection/AssessmentsAndAudits/styles.jssrc/react/src/components/ProductionLocation/PartnerSection/Certifications/index.jsxsrc/react/src/components/ProductionLocation/PartnerSection/Certifications/styles.jssrc/react/src/components/ProductionLocation/PartnerSection/Emissions/index.jsxsrc/react/src/components/ProductionLocation/PartnerSection/Emissions/styles.jssrc/react/src/components/ProductionLocation/PartnerSection/ParentSectionItem/index.jsxsrc/react/src/components/ProductionLocation/PartnerSection/ParentSectionItem/styles.jssrc/react/src/components/ProductionLocation/PartnerSection/PartnerDataContainer/index.jsxsrc/react/src/components/ProductionLocation/PartnerSection/PartnerDataContainer/styles.jssrc/react/src/components/ProductionLocation/ProductionLocationDetails/index.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetails/styles.jssrc/react/src/components/ProductionLocation/ProductionLocationDetailsContainer/index.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsContainer/styles.jssrc/react/src/components/ProductionLocation/ProductionLocationDetailsContent/index.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsContent/styles.jssrc/react/src/components/ProductionLocation/ProductionLocationDetailsGeneralFields/index.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsGeneralFields/styles.jssrc/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/index.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsLocation/styles.jssrc/react/src/components/ProductionLocation/ProductionLocationDetailsMap/index.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsMap/styles.jssrc/react/src/components/ProductionLocation/Sidebar/BackToSearch/index.jsxsrc/react/src/components/ProductionLocation/Sidebar/BackToSearch/styles.jssrc/react/src/components/ProductionLocation/Sidebar/ContributeFields/index.jsxsrc/react/src/components/ProductionLocation/Sidebar/ContributeFields/styles.jssrc/react/src/components/ProductionLocation/Sidebar/NavBar/index.jsxsrc/react/src/components/ProductionLocation/Sidebar/NavBar/styles.jssrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/index.jsxsrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/styles.js
src/react/src/components/ProductionLocation/Heading/ClosureStatus/PrimaryText.jsx
Show resolved
Hide resolved
...act/src/components/ProductionLocation/PartnerSection/ParentSectionItem/ParentSectionItem.jsx
Show resolved
Hide resolved
...act/src/components/ProductionLocation/PartnerSection/ParentSectionItem/ParentSectionItem.jsx
Show resolved
Hide resolved
...c/components/ProductionLocation/PartnerSection/PartnerDataContainer/PartnerDataContainer.jsx
Show resolved
Hide resolved
...ProductionLocation/ProductionLocationDetailsContainer/ProductionLocationDetailsContainer.jsx
Show resolved
Hide resolved
protsack-stephan
previously approved these changes
Feb 24, 2026
Collaborator
protsack-stephan
left a comment
protsack-stephan
left a comment
There was a problem hiding this comment.
Good work, one small comment. That's just for the future, feel free to ignore.
src/react/src/components/ProductionLocation/Heading/ClosureStatus/ClosureStatus.jsx
Outdated
Show resolved
Hide resolved
vlad-shapik
requested changes
Feb 24, 2026
|
protsack-stephan
approved these changes
Feb 24, 2026
Contributor
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Auto-approval is disabled; enable |
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-2353
Add basic components for new production location page.
ProductionLocationfolder structure: