Fix: #2227 Added Documents Catalog Plugin. #2263
Conversation
Summary of ChangesHello @anup39, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature: the Documents Catalog Plugin. This plugin empowers users to discover and integrate various document types directly into their maps as interactive layers. The initial implementation provides core functionality for browsing and adding documents, enhancing the application's capability to visualize diverse geospatial and non-geospatial information. While the basic UI and data fetching are in place, future work will focus on completing advanced filtering capabilities and comprehensive unit testing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Documents Catalog Plugin, enabling users to browse documents and add them to the map as a new layer. The implementation is comprehensive, covering API integration, a new React component for the catalog UI, Redux epics for state management, and necessary configuration updates. While the overall structure is good, I've identified a critical security vulnerability related to potential Cross-Site Scripting (XSS) that must be addressed. Additionally, there are several opportunities for improvement in terms of code correctness, maintainability, and removing unused code. The PR description correctly notes that filter functionality and unit tests are still pending.
| <iframe | ||
| src="\${properties['embed_url']}" | ||
| style="width: 100%; height: 500px; border: 1px solid #ddd; border-radius: 4px;" | ||
| frameborder="0" | ||
| allowfullscreen> | ||
| </iframe> |
There was a problem hiding this comment.
The featureInfo template creates an iframe with its src attribute set directly from properties['embed_url']. If an attacker can control the embed_url of a document, they can inject a malicious page, leading to a stored Cross-Site Scripting (XSS) vulnerability. You must add the sandbox attribute to the iframe to mitigate this risk. For example: sandbox="allow-scripts allow-same-origin". Adjust the sandbox permissions based on the minimum required functionality for the embedded content.
| <iframe | |
| src="\${properties['embed_url']}" | |
| style="width: 100%; height: 500px; border: 1px solid #ddd; border-radius: 4px;" | |
| frameborder="0" | |
| allowfullscreen> | |
| </iframe> | |
| <iframe | |
| src="\${properties['embed_url']}" | |
| style="width: 100%; height: 500px; border: 1px solid #ddd; border-radius: 4px;" | |
| frameborder="0" | |
| sandbox="allow-scripts allow-same-origin" | |
| allowfullscreen> | |
| </iframe> |
| <InputControl | ||
| placeholder={placeholderId} | ||
| value={q} | ||
| debounceTime={300} | ||
| onChange={(value) => setQ(value)} | ||
| /> |
There was a problem hiding this comment.
The InputControl component's placeholder prop is being assigned placeholderId, which is a message ID string. This will display the ID itself (e.g., "gnviewer.resourcesCatalogFilterPlaceholder") instead of the translated message. You should use the localizedProps enhancer on InputControl or otherwise ensure the translated string is passed.
| ...(sort && { sort: isArray(sort) ? sort : [ sort ]}), | ||
| page, | ||
| page_size: pageSize, | ||
| // api_preset: API_PRESET.DOCUMENTS |
There was a problem hiding this comment.
The api_preset: API_PRESET.DOCUMENTS is commented out. Is this intentional? If it's for future work, consider adding a // TODO: comment explaining why it's currently disabled. If it's no longer needed, it should be removed to avoid confusion.
| const oneOfTheOther = (control) => { | ||
| if (control === 'rightOverlay') return null; | ||
|
|
||
| // Handle three-way alternates | ||
| if (control === 'annotations') { | ||
| return { | ||
| control, | ||
| alternates: ['datasetsCatalog', 'documentsCatalog'] | ||
| }; | ||
| } | ||
| if (control === 'datasetsCatalog') { | ||
| return { | ||
| control, | ||
| alternates: ['annotations', 'documentsCatalog'] | ||
| }; | ||
| } | ||
| if (control === 'documentsCatalog') { | ||
| return { | ||
| control, | ||
| alternates: ['annotations', 'datasetsCatalog'] | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| control, | ||
| alternate: control === 'annotations' ? 'datasetsCatalog' : 'annotations' | ||
| alternates: [] | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The oneOfTheOther function contains several repetitive if blocks. This can be refactored to be more concise and easier to maintain, especially if more panels are added in the future.
const oneOfTheOther = (control) => {
if (control === 'rightOverlay') return null;
const panels = ['annotations', 'datasetsCatalog', 'documentsCatalog'];
if (panels.includes(control)) {
return {
control,
alternates: panels.filter(p => p !== control)
};
}
return {
control,
alternates: []
};
};| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| import React, { useRef, useState, useEffect, Component } from 'react'; |
There was a problem hiding this comment.
The Component is imported from React but is not used in this functional component. It should be removed to keep the imports clean.
| import React, { useRef, useState, useEffect, Component } from 'react'; | |
| import React, { useRef, useState, useEffect } from 'react'; |
| style={{ | ||
| position: 'absolute', | ||
| top: '10px', | ||
| left: '10px', | ||
| zIndex: 10, | ||
| backgroundColor: 'white', | ||
| borderRadius: '4px', | ||
| padding: '4px', | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| boxShadow: '0 2px 4px rgba(0,0,0,0.1)' | ||
| }} |
There was a problem hiding this comment.
There are several inline styles applied to this div. It's better to move these styles to a LESS file (e.g., _resources-catalog.less) to improve maintainability and allow for easier theming. You could create a new class like .gn-resources-catalog-item-checkbox and move the styles there.
| DocumentsCompactCatalog.propTypes = { | ||
| request: PropTypes.func, | ||
| responseToEntries: PropTypes.func, | ||
| pageSize: PropTypes.number, | ||
| placeholderId: PropTypes.string, | ||
| addSearchAsLayer: PropTypes.string, | ||
| selectAll: PropTypes.string, | ||
| onClose: PropTypes.func, | ||
| onSelect: PropTypes.func, | ||
| titleId: PropTypes.string, | ||
| noResultId: PropTypes.string | ||
| }; |
There was a problem hiding this comment.
The component is missing propTypes validation for style, loading (as resourceLoading), and params. Please add them for better component contract and debugging.
DocumentsCompactCatalog.propTypes = {
// ... existing propTypes
style: PropTypes.object,
loading: PropTypes.bool,
params: PropTypes.object
// ...
};| function DocumentsCatalog({ | ||
| onAdd, | ||
| onUpdate, | ||
| onZoomTo, | ||
| existingLayers, | ||
| ...props | ||
| }) { |
There was a problem hiding this comment.
The DocumentsCatalog component receives onUpdate and existingLayers as props, but they are not used within the component. If they are not needed, they should be removed from the component's props and from the connect call to avoid passing down unused properties.
| const unId= uuid(); | ||
|
|
||
| return Promise.all(documentPromises).then(fullDocs => { | ||
| const extendedParams = {}; |
There was a problem hiding this comment.
The extendedParams variable is initialized as an empty object and passed to the layer configuration, but it's never modified. It appears to be unused and should be removed.
2 participants
This PR add:
Remaining:
Screenshots:
Image1:
Image:2
Image:3
Image:4
Image: 5