[WIP] : DO NOT MERGE! Kpmp 6434 web component#616
Conversation
WalkthroughThe changes integrate HubMap's medical illustration web component into the application. External CDN resources (stylesheet and JavaScript module) are loaded in the HTML head. A new route Changes
✨ Finishing touches
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: 1
🧹 Nitpick comments (3)
src/components/Explorer/HubMapSchema.js (3)
24-30: Consider making illustration URLs configurable.The URLs on lines 27-28 are hard-coded, which reduces flexibility for different environments or use cases. Consider accepting these as props or loading them from environment variables.
♻️ Proposed refactor to use props with defaults
-function HubMapSchema(props) { +function HubMapSchema({ + selectedIllustration = "https://purl.humanatlas.io/2d-ftu/kidney-renal-corpuscle", + illustrations = "https://cdn.humanatlas.io/digital-objects/graph/2d-ftu-illustrations/latest/assets/2d-ftu-illustrations.jsonld" +}) { const ref = useRef(null); useEffect(() => { // Manually attach event listeners or call imperative methods if needed const element = ref.current; if (!element) return; const handleClick = (event) => { console.log('HubMapSchema clicked', event.target.id); }; element.addEventListener('click', handleClick); return () => { element.removeEventListener('click', handleClick); }; }, []); // Pass properties as attributes (which are treated as strings) return ( <hra-medical-illustration ref={ref} - selected-illustration="https://purl.humanatlas.io/2d-ftu/kidney-renal-corpuscle" - illustrations="https://cdn.humanatlas.io/digital-objects/graph/2d-ftu-illustrations/latest/assets/2d-ftu-illustrations.jsonld" + selected-illustration={selectedIllustration} + illustrations={illustrations} /> ); }
11-13: Is this console logging temporary?The click handler only logs to the console without any functional logic. If this is temporary debugging code, consider removing it before merging. If you need to implement actual click handling logic, consider what action should be taken.
If you need help implementing click handling logic (e.g., navigation, state updates, or analytics tracking), I can provide suggestions.
34-43: Consider removing the unnecessary class wrapper.The
HubMapSchemaComponentclass simply renders the functional component without adding any additional logic. You can export the functional component directly.♻️ Proposed simplification
-class HubMapSchemaComponent extends Component { - - render() { - return ( - <HubMapSchema /> - ); - } -} - -export default HubMapSchemaComponent; +export default HubMapSchema;Then update the import in
src/App.js:-import HubMapSchemaComponent from './components/Explorer/HubMapSchema'; +import HubMapSchema from './components/Explorer/HubMapSchema';And the route:
-<Route exact path="/explorer/hubmap" component={HubMapSchemaComponent} /> +<Route exact path="/explorer/hubmap" component={HubMapSchema} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/index.htmlsrc/App.jssrc/components/Explorer/HubMapSchema.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.19.0)
🔇 Additional comments (4)
src/App.js (2)
28-28: LGTM!The import is correctly structured and follows the established naming conventions in the file.
86-86: Store prop omission is correct.
HubMapSchemaComponentdoes not use Redux and has no need for store access. The route configuration is appropriate as-is.src/components/Explorer/HubMapSchema.js (1)
1-1: LGTM!All imports are correctly specified and utilized in the component.
public/index.html (1)
33-35: Code follows official HRA documentation; SRI consideration is optional hardening.This implementation matches the official medical-illustration embedding guide from HubMAP's HRA UI repository. However, consider these optional security improvements:
- SRI hashes are not documented by HRA but can be added if you require strict integrity verification. Note that this requires manual hash computation and will break if HRA updates the resources (they appear to be continuously deployed without fixed version releases).
- A local fallback or bundled copy could improve resilience if cdn.humanatlas.io becomes temporarily unavailable, though this is a design trade-off for external component usage.
If you choose to implement stricter security than the official guidance recommends, document the decision and establish a maintenance process for updating SRI hashes when resources change.
| useEffect(() => { | ||
| // Manually attach event listeners or call imperative methods if needed | ||
| const element = ref.current; | ||
| if (!element) return; | ||
|
|
||
| const handleClick = (event) => { | ||
| console.log('HubMapSchema clicked', event.explicitOriginalTarget.id); | ||
| }; | ||
|
|
||
| element.addEventListener('click', handleClick); | ||
|
|
||
| return () => { | ||
| element.removeEventListener('click', handleClick); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Critical browser compatibility issue: explicitOriginalTarget is Firefox-only.
Line 12 uses event.explicitOriginalTarget, which is a non-standard Firefox-only property. This will cause runtime errors in Chrome, Safari, and Edge browsers where this property is undefined.
🔧 Proposed fix for cross-browser compatibility
const handleClick = (event) => {
- console.log('HubMapSchema clicked', event.explicitOriginalTarget.id);
+ console.log('HubMapSchema clicked', event.target.id);
};Alternatively, if you need to access the original target within a shadow DOM:
const handleClick = (event) => {
- console.log('HubMapSchema clicked', event.explicitOriginalTarget.id);
+ const target = event.composedPath?.()?.[0] || event.target;
+ console.log('HubMapSchema clicked', target.id);
};1 participant
Summary by CodeRabbit
/explorer/hubmaproute for accessing the schema visualization✏️ Tip: You can customize this high-level summary in your review settings.