[OSDEV-2367] Implement "Supply Chain Network" section with contributors drawer#907
Open
[OSDEV-2367] Implement "Supply Chain Network" section with contributors drawer#907
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a stateful SupplyChain UI, a new SupplyChainNetworkDrawer (with styles, constants, and utils), comprehensive tests for drawer and contributor behavior, and passes Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SupplyChain
participant Drawer as SupplyChainNetworkDrawer
participant MUIDrawer as Material-UI Drawer
User->>SupplyChain: Click "View all data sources"
SupplyChain->>SupplyChain: set open = true
SupplyChain->>Drawer: props(open=true, totalCount, typeCounts, publicContributors, nonPublicContributors)
Drawer->>MUIDrawer: render right-anchored drawer with content
MUIDrawer->>User: display title, info box, lists, anonymized section
User->>MUIDrawer: click close/backdrop
MUIDrawer->>Drawer: onClose event
Drawer->>SupplyChain: invoke onClose callback
SupplyChain->>SupplyChain: set open = false
SupplyChain->>Drawer: props(open=false)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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: 3
🧹 Nitpick comments (6)
src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx (1)
130-138: Reconsider usingOpenInNewIconfor internal navigation.The
OpenInNewIcontypically indicates a link that opens in a new browser tab/window. However, thisLinkcomponent performs internal SPA navigation to the profile page. This could confuse users expecting the link to open externally.Consider either:
- Removing the icon for internal links
- Using a different icon (e.g.,
ArrowForwardIconorChevronRightIcon)- Keeping the icon if the intent is to communicate "navigates away from current view"
🤖 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/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx` around lines 130 - 138, The Link in SupplyChainNetworkDrawer.jsx uses OpenInNewIcon which implies opening an external/new tab but the Link performs internal SPA navigation; update the UI to avoid confusing users by either removing OpenInNewIcon or replacing it with a more appropriate icon (e.g., ArrowForwardIcon or ChevronRightIcon). To fix, modify the JSX inside the contributor Link (the element using makeProfileRouteLink(contributor.id) and classes.contributorName) to remove or replace the <OpenInNewIcon .../> and update the imports accordingly (remove OpenInNewIcon import and import ArrowForwardIcon/ChevronRightIcon if chosen) and ensure styles for classes.externalLinkIcon still apply or are renamed/removed.src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/styles.js (2)
103-108: Verify unitless gap value behavior.The
gap: 6value lacks a unit. While Material-UI's JSS typically interprets numbers as pixels, explicit units (e.g.,'6px') would be clearer and consistent with other gap definitions in this file (line 27 uses'8px').♻️ Suggested fix
typeSummary: Object.freeze({ display: 'flex', flexWrap: 'wrap', - gap: 6, + gap: '6px', marginBottom: '16px', }),🤖 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/SupplyChainNetworkDrawer/styles.js` around lines 103 - 108, The style object typeSummary uses a unitless gap value (gap: 6) which relies on JSS number-to-pixel coercion; update typeSummary to use an explicit unit (gap: '6px') to match the file's other explicit gap definitions (e.g., the '8px' usage) and ensure consistent, clear styling in the styles.js definition for typeSummary.
62-67: Consider extracting hardcoded color to COLOURS constant.The border color
#C0DBFEis hardcoded here whileCOLOURS.EXTRA_LIGHT_BLUEis used for the background. For consistency and maintainability, consider adding this border color to the COLOURS utility.🤖 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/SupplyChainNetworkDrawer/styles.js` around lines 62 - 67, The infoBox style in styles.js uses a hardcoded border color "#C0DBFE"; extract this value into the COLOURS utility (e.g., add COLOURS.EXTRA_LIGHT_BLUE_BORDER or similar) and replace the literal in infoBox.border with the new COLOURS constant; ensure you export the new constant from the COLOURS module and update any imports if necessary so infoBox now references COLOURS.<NEW_NAME> instead of the hex string.src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChain.jsx (1)
30-48: Consider optimizing aggregateByType for larger datasets.The current implementation creates a new array on each iteration when updating an existing type's count (line 38-42). For small datasets this is fine, but could be optimized using a Map:
♻️ Optional optimization
const aggregateByType = nonPublicContributors => - nonPublicContributors - .filter(c => c.contributor_type != null) - .reduce((acc, c) => { - const existing = acc.find( - x => x.contributor_type === c.contributor_type, - ); - if (existing) { - return acc.map(item => - item.contributor_type === c.contributor_type - ? { ...item, count: item.count + (c.count || 1) } - : item, - ); - } - return [ - ...acc, - { contributor_type: c.contributor_type, count: c.count || 1 }, - ]; - }, []); + { + const map = new Map(); + nonPublicContributors + .filter(c => c.contributor_type != null) + .forEach(c => { + const type = c.contributor_type; + map.set(type, (map.get(type) || 0) + (c.count || 1)); + }); + return Array.from(map, ([contributor_type, count]) => ({ + contributor_type, + count, + })); + };🤖 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/SupplyChain.jsx` around lines 30 - 48, The aggregateByType reducer is O(n^2) because it recreates arrays on each existing-type update; replace the reduce with a single-pass Map-based aggregation in aggregateByType: iterate nonPublicContributors, skip entries with null contributor_type, accumulate counts into a Map keyed by contributor_type (adding c.count || 1), then convert the Map entries back to the same array shape [{ contributor_type, count }]; ensure the function still returns an array and preserves the original count fallback logic.src/react/src/__tests__/components/SupplyChainNetwork.test.jsx (1)
156-160: Verify aria-label case sensitivity in test assertion.The component uses
aria-label="Close"(capitalized) while the test usesgetByLabelText('close')(lowercase). Testing Library'sgetByLabelTextis case-insensitive by default for accessible names, so this should work, but consider using the exact case for clarity:♻️ Optional: Match exact case
- expect(screen.getByLabelText('close')).toBeInTheDocument(); + expect(screen.getByLabelText('Close')).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/__tests__/components/SupplyChainNetwork.test.jsx` around lines 156 - 160, The assertion uses screen.getByLabelText('close') while the component sets aria-label="Close"; update the test in SupplyChainNetwork.test.jsx to match the component's casing by using screen.getByLabelText('Close') (or explicitly case-insensitive with screen.getByLabelText(/close/i)) so the expectation targets the exact accessible name; change the call replacing "close" with "Close" (or the regex) where the test currently calls getByLabelText('close').src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/constants.js (1)
1-2: Consider consolidating duplicate constants.
DRAWER_TITLEandALL_DATA_SOURCES_LABELhave identical values. If they serve different semantic purposes (e.g., one for the header, one for the section label), the current separation is fine. Otherwise, consider using a single constant to avoid drift.🤖 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/SupplyChainNetworkDrawer/constants.js` around lines 1 - 2, DRAWER_TITLE and ALL_DATA_SOURCES_LABEL both hold the same string; either consolidate them into a single exported constant (e.g., DATA_SOURCES_LABEL) and replace usages of DRAWER_TITLE and ALL_DATA_SOURCES_LABEL throughout the codebase, or if they have distinct semantics, rename them to clarify intent (e.g., DRAWER_HEADER_TITLE vs SECTION_LABEL) and ensure each is used only for its specific purpose; update imports/uses referencing DRAWER_TITLE and ALL_DATA_SOURCES_LABEL accordingly.
🤖 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/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx`:
- Around line 220-227: The publicContributors prop's shape must require the id
field (it's used as the React key and passed to makeProfileRouteLink), so update
the propTypes for publicContributors by changing the id entry in the shape from
number to number.isRequired (and optionally ensure the containing arrayOf(...)
is required if the array itself must always be present). Locate the
publicContributors propType definition (the shape with id, contributor_name,
contributor_type, list_names) and make id required so keys and calls to
makeProfileRouteLink always receive an id.
- Line 78: The Divider usage in SupplyChainNetworkDrawer.jsx is passing a
non-supported height prop (see the Divider element) so replace the height prop
with a style prop; locate the Divider JSX in the SupplyChainNetworkDrawer
component and change the attribute from height={1} to style={{ height: 1 }} so
the inline style is applied correctly by Material-UI v3.
In `@src/react/src/components/ProductionLocation/Sidebar/SupplyChain/utils.js`:
- Around line 1-8: Update the PLURAL_MAP constant to exactly match the Django
contributor types: remove the incorrect keys 'Auditor' and 'Government' from
PLURAL_MAP and add the four missing mappings with their full keys and plurals:
'Academic / Researcher / Journalist / Student' -> 'Academics / Researchers /
Journalists / Students', 'Auditor / Certification Scheme / Service Provider' ->
'Auditors / Certification Schemes / Service Providers', 'Facility / Factory /
Manufacturing Group / Supplier / Vendor' -> 'Facilities / Factories /
Manufacturing Groups / Suppliers / Vendors', and 'Union' -> 'Unions'; keep the
existing correct mappings (e.g., 'Brand / Retailer', 'Civil Society
Organization', 'Multi-Stakeholder Initiative', 'Other') intact so PLURAL_MAP
exactly mirrors the backend contributor types used by the code that references
PLURAL_MAP.
---
Nitpick comments:
In `@src/react/src/__tests__/components/SupplyChainNetwork.test.jsx`:
- Around line 156-160: The assertion uses screen.getByLabelText('close') while
the component sets aria-label="Close"; update the test in
SupplyChainNetwork.test.jsx to match the component's casing by using
screen.getByLabelText('Close') (or explicitly case-insensitive with
screen.getByLabelText(/close/i)) so the expectation targets the exact accessible
name; change the call replacing "close" with "Close" (or the regex) where the
test currently calls getByLabelText('close').
In
`@src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChain.jsx`:
- Around line 30-48: The aggregateByType reducer is O(n^2) because it recreates
arrays on each existing-type update; replace the reduce with a single-pass
Map-based aggregation in aggregateByType: iterate nonPublicContributors, skip
entries with null contributor_type, accumulate counts into a Map keyed by
contributor_type (adding c.count || 1), then convert the Map entries back to the
same array shape [{ contributor_type, count }]; ensure the function still
returns an array and preserves the original count fallback logic.
In
`@src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/constants.js`:
- Around line 1-2: DRAWER_TITLE and ALL_DATA_SOURCES_LABEL both hold the same
string; either consolidate them into a single exported constant (e.g.,
DATA_SOURCES_LABEL) and replace usages of DRAWER_TITLE and
ALL_DATA_SOURCES_LABEL throughout the codebase, or if they have distinct
semantics, rename them to clarify intent (e.g., DRAWER_HEADER_TITLE vs
SECTION_LABEL) and ensure each is used only for its specific purpose; update
imports/uses referencing DRAWER_TITLE and ALL_DATA_SOURCES_LABEL accordingly.
In
`@src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/styles.js`:
- Around line 103-108: The style object typeSummary uses a unitless gap value
(gap: 6) which relies on JSS number-to-pixel coercion; update typeSummary to use
an explicit unit (gap: '6px') to match the file's other explicit gap definitions
(e.g., the '8px' usage) and ensure consistent, clear styling in the styles.js
definition for typeSummary.
- Around line 62-67: The infoBox style in styles.js uses a hardcoded border
color "#C0DBFE"; extract this value into the COLOURS utility (e.g., add
COLOURS.EXTRA_LIGHT_BLUE_BORDER or similar) and replace the literal in
infoBox.border with the new COLOURS constant; ensure you export the new constant
from the COLOURS module and update any imports if necessary so infoBox now
references COLOURS.<NEW_NAME> instead of the hex string.
In
`@src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx`:
- Around line 130-138: The Link in SupplyChainNetworkDrawer.jsx uses
OpenInNewIcon which implies opening an external/new tab but the Link performs
internal SPA navigation; update the UI to avoid confusing users by either
removing OpenInNewIcon or replacing it with a more appropriate icon (e.g.,
ArrowForwardIcon or ChevronRightIcon). To fix, modify the JSX inside the
contributor Link (the element using makeProfileRouteLink(contributor.id) and
classes.contributorName) to remove or replace the <OpenInNewIcon .../> and
update the imports accordingly (remove OpenInNewIcon import and import
ArrowForwardIcon/ChevronRightIcon if chosen) and ensure styles for
classes.externalLinkIcon still apply or are renamed/removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dfe6026c-cb01-42fe-80f8-e2cc71fa5934
📒 Files selected for processing (8)
src/react/src/__tests__/components/SupplyChainNetwork.test.jsxsrc/react/src/components/ProductionLocation/ProductionLocationDetailsContainer/ProductionLocationDetailsContainer.jsxsrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChain.jsxsrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsxsrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/constants.jssrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/styles.jssrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/styles.jssrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/utils.js
...ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx
Outdated
Show resolved
Hide resolved
...ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx
Show resolved
Hide resolved
src/react/src/components/ProductionLocation/Sidebar/SupplyChain/utils.js
Show resolved
Hide resolved
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
…n/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 55.73%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.75%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
tana-bee
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 42.42%Your code coverage diff: 0.49% ▴ ✅ All code changes are covered |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx`:
- Around line 130-137: Remove the misleading external-link affordance: inside
the Link that uses makeProfileRouteLink(contributor.id) (and styled with
classes.contributorName), delete the OpenInNewIcon element and any usage of
classes.externalLinkIcon; if a trailing visual is still desired use a
neutral/inline icon (e.g., a generic chevron or person/info icon) that does not
imply opening an external window instead of OpenInNewIcon.
- Around line 147-152: The current key for list entries inside the contributor
mapping uses `${contributor.id}-${listName}`, which can collide if the API
returns duplicate list names; update the mapping in the
contributor.list_names.map call to use a stable unique key per entry (preferably
an id on the list item if one exists, otherwise include the map index) so keys
become e.g. `${contributor.id}-${index}-${listName}` or
`${contributor.id}-${listItem.id}`; change the code that builds the key in the
map to use the chosen unique identifier instead of listName alone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e33cca0-c6f1-45fe-ad73-60bc1483e3d8
📒 Files selected for processing (3)
src/react/src/__tests__/components/SupplyChainNetwork.test.jsxsrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsxsrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/utils.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/react/src/components/ProductionLocation/Sidebar/SupplyChain/utils.js
- src/react/src/tests/components/SupplyChainNetwork.test.jsx
...ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx
Outdated
Show resolved
Hide resolved
...ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Sidebar/SupplyChain/SupplyChainNetworkDrawer/styles.js`:
- Around line 5-8: The drawerPaper style currently sets a fixed width of '390px'
which can overflow on narrow viewports; update the drawerPaper object (symbol:
drawerPaper) to use responsive sizing by replacing the fixed width with width:
'100%' and adding maxWidth: '390px' (optionally keep boxShadow as-is) so the
drawer fills small screens but is capped at 390px on larger screens.
In
`@src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx`:
- Around line 216-222: The propType for publicContributors allows
contributor_name to be optional, which can render empty links; update the shape
inside the publicContributors propType (the shape object in
SupplyChainNetworkDrawer.jsx) to make contributor_name required by changing its
declaration from string to string.isRequired so the component (rendering
contributor_name as link text) won't receive empty values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1152d872-0a2b-4496-a177-11a80575817e
📒 Files selected for processing (2)
src/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsxsrc/react/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/styles.js
...act/src/components/ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/styles.js
Show resolved
Hide resolved
...ProductionLocation/Sidebar/SupplyChain/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx
Show resolved
Hide resolved
…n/SupplyChainNetworkDrawer/styles.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…n/SupplyChainNetworkDrawer/SupplyChainNetworkDrawer.jsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Collaborator
There was a problem hiding this comment.
Great work! I’ve left a few comments. Just a reminder that it’s entirely up to you whether to implement the suggestions or resolve them as they are. We follow this engineering guide for our code reviews - specifically the section on handling comments.
Additionally, I think I misled you a bit with my previous comment regarding the borders. Here:
I forgot to mention that for this section, we should probably keep the tags rounded, as shown in the design:
Also, I noticed some missing padding on the tags inside the drawer:
Please let me know if you need any help in handling those, happy to jump on a call and discuss!
Comment on lines
+93
to
+94
| // Three contributors: two Brand / Retailers surrounding one Auditor in API order. | ||
| // After grouping the two Brand / Retailers must appear consecutively. |
Collaborator
There was a problem hiding this comment.
We usually try to follow the best practice of ensuring that comments explain why something was done a certain way, rather than how it works. The code itself should clearly communicate the 'how' through its logic and structure.
| test('renders "View all N data sources" trigger button', () => { | ||
| renderSection([publicContributor, nonPublicContributor]); | ||
|
|
||
| // totalCount = publicContributor.count(1) + nonPublicContributor.count(3) = 4 |
Collaborator
There was a problem hiding this comment.
This looks like it can be removed, can we delete this line?
| <ContributeFields osId={osID} /> | ||
| <SupplyChain /> | ||
| <SupplyChain | ||
| contributors={data?.properties?.contributors ?? []} |
Collaborator
There was a problem hiding this comment.
Instead of prop drilling, we could use Redux directly in the SupplyChain component. This would make it easier to move the component around if we need to restructure the tree later.
Comment on lines
+41
to
+204
| return ( | ||
| <Drawer | ||
| anchor="right" | ||
| open={open} | ||
| onClose={onClose} | ||
| classes={{ paper: classes.drawerPaper }} | ||
| > | ||
| <div | ||
| className={classes.drawerContent} | ||
| data-testid="supply-chain-drawer" | ||
| > | ||
| <div className={classes.header}> | ||
| <div className={classes.headerLeft}> | ||
| <PeopleOutlineIcon className={classes.titleIcon} /> | ||
| <Typography className={classes.title} component="h2"> | ||
| {DRAWER_TITLE} | ||
| </Typography> | ||
| </div> | ||
| <IconButton | ||
| className={classes.closeButton} | ||
| aria-label="Close" | ||
| onClick={onClose} | ||
| data-testid="supply-chain-drawer-close" | ||
| > | ||
| <CloseIcon /> | ||
| </IconButton> | ||
| </div> | ||
|
|
||
| <Typography className={classes.subtitle} component="p"> | ||
| {totalCount}{' '} | ||
| {totalCount === 1 | ||
| ? 'organization has' | ||
| : 'organizations have'}{' '} | ||
| shared data about this production location | ||
| </Typography> | ||
|
|
||
| <Divider style={{ height: 1 }} /> | ||
|
|
||
| <Typography className={classes.sectionLabel} component="p"> | ||
| {allSourcesLabel} | ||
| </Typography> | ||
|
|
||
| <div className={classes.infoBox}> | ||
| <div className={classes.infoBoxWithIcon}> | ||
| <InfoOutlinedIcon className={classes.infoIcon} /> | ||
| <div className={classes.infoBoxContent}> | ||
| <Typography | ||
| className={classes.infoText} | ||
| component="div" | ||
| > | ||
| {INFO_TEXT} | ||
| </Typography> | ||
| <a | ||
| href={LEARN_MORE_URL} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className={classes.learnMoreLink} | ||
| > | ||
| {LEARN_MORE_LABEL} | ||
| <span className={classes.learnMoreArrow}> | ||
| → | ||
| </span> | ||
| </a> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| {typeCounts.length > 0 && ( | ||
| <div className={classes.typeSummary}> | ||
| {typeCounts.map(({ type, count }) => ( | ||
| <Typography | ||
| key={type} | ||
| className={classes.typeChip} | ||
| component="p" | ||
| > | ||
| <strong>{count}</strong>{' '} | ||
| {pluralizeContributorType(type, count)} | ||
| </Typography> | ||
| ))} | ||
| </div> | ||
| )} | ||
|
|
||
| <div className={classes.listScroll}> | ||
| {publicContributors.map(contributor => ( | ||
| <div | ||
| key={contributor.id} | ||
| className={classes.contributorEntry} | ||
| > | ||
| <Link | ||
| to={makeProfileRouteLink(contributor.id)} | ||
| className={classes.contributorName} | ||
| > | ||
| {contributor.contributor_name} | ||
| </Link> | ||
| {contributor.contributor_type && ( | ||
| <Typography | ||
| className={classes.contributorType} | ||
| component="p" | ||
| > | ||
| {contributor.contributor_type} | ||
| </Typography> | ||
| )} | ||
| {contributor.list_names && | ||
| contributor.list_names | ||
| .map((name, i) => ({ | ||
| name, | ||
| key: `${contributor.id}-${i}`, | ||
| })) | ||
| .filter(({ name }) => name) | ||
| .map(({ name: listName, key }) => ( | ||
| <div | ||
| key={key} | ||
| className={classes.listEntry} | ||
| > | ||
| <Typography | ||
| className={ | ||
| classes.listEntryLabel | ||
| } | ||
| component="p" | ||
| > | ||
| <ListIcon | ||
| className={classes.listIcon} | ||
| /> | ||
| {UPLOADED_VIA_LIST_LABEL} | ||
| </Typography> | ||
| <Typography | ||
| className={classes.listName} | ||
| component="p" | ||
| > | ||
| {listName} | ||
| </Typography> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| ))} | ||
| </div> | ||
|
|
||
| {nonPublicContributors.length > 0 && ( | ||
| <> | ||
| <Typography | ||
| className={classes.sectionLabel} | ||
| component="p" | ||
| > | ||
| {ANONYMIZED_SECTION_TITLE} | ||
| </Typography> | ||
| {nonPublicContributors.map(contributor => ( | ||
| <Typography | ||
| key={contributor.contributor_type} | ||
| className={classes.anonymizedType} | ||
| component="p" | ||
| > | ||
| {contributor.count}{' '} | ||
| {pluralizeContributorType( | ||
| contributor.contributor_type, | ||
| contributor.count, | ||
| )} | ||
| </Typography> | ||
| ))} | ||
| </> | ||
| )} | ||
| </div> | ||
| </Drawer> | ||
| ); | ||
| }; |
Collaborator
There was a problem hiding this comment.
The rendering logic is quite extensive; I wonder if we could break this down into multiple sub-components. That would make this component much easier to maintain.
Comment on lines
+16
to
+51
| const buildTypeCounts = contributors => { | ||
| const totals = contributors.reduce((acc, contributor) => { | ||
| const type = contributor.contributor_type; | ||
| if (!type) return acc; | ||
| const count = contributor.count || 1; | ||
| acc[type] = (acc[type] || 0) + count; | ||
| return acc; | ||
| }, {}); | ||
|
|
||
| return Object.entries(totals).map(([type, count]) => ({ type, count })); | ||
| }; | ||
|
|
||
| // Aggregate non-public contributors by type so each type appears only once. | ||
| // This prevents duplicate React keys and duplicate rows in the drawer. | ||
| const aggregateByType = nonPublicContributors => | ||
| nonPublicContributors | ||
| .filter(c => c.contributor_type != null) | ||
| .reduce((acc, c) => { | ||
| const existing = acc.find( | ||
| x => x.contributor_type === c.contributor_type, | ||
| ); | ||
| if (existing) { | ||
| return acc.map(item => | ||
| item.contributor_type === c.contributor_type | ||
| ? { ...item, count: item.count + (c.count || 1) } | ||
| : item, | ||
| ); | ||
| } | ||
| return [ | ||
| ...acc, | ||
| { contributor_type: c.contributor_type, count: c.count || 1 }, | ||
| ]; | ||
| }, []); | ||
|
|
||
| const getTotalCount = contributors => | ||
| contributors.reduce((sum, c) => sum + (c.count || 1), 0); |
Collaborator
There was a problem hiding this comment.
I wonder if we could move these computations to the reducer. By processing the data once upon fetching, we can avoid redundant calculations and prevent blocking the main thread during rendering.
Also we could potentially use the useMemo hook when calling those computations, as an alternative.
@vlad-shapik @VadimKovalenkoSNF Any suggestions here? Am I missing something?
Collaborator
There was a problem hiding this comment.
Additionally, those functions should be easy to unit test independently. It would be a good idea to add tests now to ensure we don't introduce regressions when we modify them in the future.
Comment on lines
+70
to
+83
| const visibleContributors = contributors.filter( | ||
| c => !!c.contributor_name || !!c.contributor_type, | ||
| ); | ||
|
|
||
| if (!visibleContributors.length) return null; | ||
|
|
||
| const { | ||
| publicContributors, | ||
| nonPublicContributors, | ||
| } = splitContributorsIntoPublicAndNonPublic(visibleContributors); | ||
|
|
||
| const sortedPublicContributors = [...publicContributors].sort((a, b) => | ||
| (a.contributor_type || '').localeCompare(b.contributor_type || ''), | ||
| ); |
Collaborator
There was a problem hiding this comment.
This could potentially block the component from rendering until the computation completes. While usually not an issue for small tasks, it would be a significant improvement to either pre-calculate the data in the reducer or use the useMemo hook to minimize re-computation during renders.
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
2 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.
Description:
Summary
https://opensupplyhub.atlassian.net/browse/OSDEV-2367
As part of the Production Location page redesign, this PR implements the Supply Chain
Network section in the sidebar.
What's new:
Auditor"), using pluralized type labels
location")
list names
Code changes
drawer open/close state with focus-return accessibility handling
pattern from OSDEV-2370
drawer open/close, and anonymized contributor handling
Dependencies
api/facilities/{os_id}/ response
OSDEV-2370 (not yet merged); once merged, the inline InfoBox markup in the drawer can be
replaced with a direct import