feat: UX & UI impros in various places (evidence cards, voting cards, case overview, top jurors) #1754
Conversation
…tached, transaction hash
WalkthroughThe changes in this pull request introduce a new field, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (4)
web/src/hooks/queries/useEvidences.ts (1)
Line range hint
41-57: Consider caching optimization for search queries.The implementation is solid, but search queries might benefit from a shorter cache lifetime to prevent stale results.
Consider adding a separate staleTime configuration for search queries:
return useQuery<{ evidences: EvidenceDetailsFragment[] }>({ queryKey: [keywords ? `evidenceSearchQuery${evidenceGroup}-${keywords}` : `evidencesQuery${evidenceGroup}`], enabled: isEnabled, refetchInterval: REFETCH_INTERVAL, + staleTime: keywords ? 0 : REFETCH_INTERVAL, // Immediate refetch for search queries queryFn: async () => {web/src/pages/Cases/CaseDetails/Evidence/index.tsx (2)
98-107: LGTM! Consider adding type safety.The addition of
transactionHashto the evidence mapping is correct and aligns with the PR objectives. However, consider adding TypeScript types for the evidence object to ensure type safety.interface Evidence { evidence: string; sender: { id: string }; timestamp: string; transactionHash: string; name: string; description: string; fileURI: string; evidenceIndex: string; }
116-121: Consider extracting the evidence card rendering logic to reduce duplication.The evidence card rendering logic is duplicated between real and spam evidences. Consider extracting this into a reusable function to improve maintainability.
const renderEvidenceCard = ({ evidence, sender, timestamp, transactionHash, name, description, fileURI, evidenceIndex, }) => ( <EvidenceCard key={timestamp} index={parseInt(evidenceIndex)} sender={sender?.id} {...{ evidence, timestamp, transactionHash, name, description, fileURI }} /> );Then use it like:
evidences?.spamEvidences.map(renderEvidenceCard)web/src/components/EvidenceCard.tsx (1)
179-185: Enhance accessibility for clickable elementsConsider adding:
- Visual indicators for clickable elements in their default state
- Descriptive aria-labels for screen readers
- <StyledA href={addressExplorerLink} rel="noreferrer" target="_blank"> + <StyledA + href={addressExplorerLink} + rel="noreferrer" + target="_blank" + aria-label={`View address ${sender} on block explorer`} + > <p>{shortenAddress(sender)}</p> </StyledA> - <StyledA href={transactionExplorerLink} rel="noreferrer" target="_blank"> + <StyledA + href={transactionExplorerLink} + rel="noreferrer" + target="_blank" + aria-label={`View transaction from ${formatDate(Number(timestamp), true)} on block explorer`} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
subgraph/core/schema.graphql(1 hunks)subgraph/core/src/EvidenceModule.ts(1 hunks)web/src/components/EvidenceCard.tsx(5 hunks)web/src/hooks/queries/useEvidences.ts(1 hunks)web/src/pages/Cases/CaseDetails/Evidence/index.tsx(2 hunks)
🔇 Additional comments (6)
web/src/hooks/queries/useEvidences.ts (1)
18-18: LGTM! Verify fragment usage across components.
The addition of transactionHash to the fragment is correct and well-positioned. This field enables the new transaction explorer linking feature.
Let's verify the fragment usage across components:
✅ Verification successful
LGTM! Fragment usage is consistent and well-integrated
The verification shows that:
- The
EvidenceDetailsfragment is properly defined inuseEvidences.tsand includes thetransactionHashfield - The fragment is consistently used in evidence queries within the same file
- The
transactionHashfield is correctly consumed inEvidenceCard.tsxandEvidence/index.tsxcomponents for displaying transaction links - No direct queries of
transactionHashoutside the fragment were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the EvidenceDetails fragment is consistently used across components
# and that no component is directly querying transactionHash without using the fragment
# Check for fragment usage
rg "\.\.\.EvidenceDetails" -A 3
# Check for direct transactionHash queries (should not find any)
rg "transactionHash[^.]" --type ts --type tsx
Length of output: 494
Script:
#!/bin/bash
# Let's try a more comprehensive search approach
# 1. Find all TypeScript files that might use the fragment
fd "\.(ts|tsx)$" web/src
# 2. Check for EvidenceDetails fragment usage with better context
rg "\.\.\.EvidenceDetails" -B 2 -A 5 --type-add "typescript:*.{ts,tsx}"
# 3. Check for transactionHash usage in TypeScript files
rg "transactionHash" --type-add "typescript:*.{ts,tsx}" --type typescript
# 4. Look for GraphQL fragment definition
rg "fragment EvidenceDetails" --type-add "typescript:*.{ts,tsx}" --type typescript -B 2 -A 10
Length of output: 19970
subgraph/core/src/EvidenceModule.ts (2)
Line range hint 28-53: LGTM! Robust error handling preserved.
The error handling ensures the evidence record (including the new transaction hash) is saved in all scenarios, with proper logging of failure cases.
20-20: LGTM! Verify schema compatibility.
The transaction hash assignment is correctly placed with other metadata assignments and will be properly saved regardless of JSON parsing outcomes.
Let's verify the schema definition matches the implementation:
✅ Verification successful
Schema compatibility verified, implementation is correct
The transactionHash field is properly defined in the schema as Bytes! (non-nullable), which matches the implementation where we assign event.transaction.hash. The field placement and type are consistent across both the schema and the event handler.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ClassicEvidence entity schema definition
# Expected: Should find transactionHash field of type Bytes! in schema.graphql
rg -A 1 'type ClassicEvidence' subgraph/core/schema.graphql
Length of output: 257
Script:
#!/bin/bash
# Let's get more context around the ClassicEvidence entity definition
rg -A 15 'type ClassicEvidence implements Evidence' subgraph/core/schema.graphql
Length of output: 514
web/src/components/EvidenceCard.tsx (2)
1-1: LGTM: Clean styled-component implementation
The new StyledA component follows consistent styling patterns and properly handles hover states using theme colors.
Also applies to: 12-12, 120-131
179-185: LGTM: Clean implementation of clickable elements
The implementation:
- Makes addresses and timestamps clickable as per PR objectives
- Follows security best practices with rel="noreferrer"
- Maintains existing functionality while adding new features
subgraph/core/schema.graphql (1)
321-321: Verify data migration strategy for existing evidence entries.
Since ClassicEvidence is marked as immutable and transactionHash is a required field, ensure there's a migration strategy to handle existing evidence entries.
Run the following script to check for existing evidence entries:
…nding court when clicking on it
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/src/components/DisputeView/DisputeInfo/DisputeInfoCard.tsx (1)
70-77: Consider accessibility improvements for the styled linkWhile the hover state is well implemented, consider adding the following improvements:
const StyledLink = styled(Link)` + text-decoration: none; :hover { label { &.value { cursor: pointer; color: ${({ theme }) => theme.primaryBlue}; text-decoration: underline; } } } + :focus-visible { + outline: 2px solid ${({ theme }) => theme.primaryBlue}; + outline-offset: 2px; + } `;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/src/components/DisputeView/DisputeInfo/DisputeInfoCard.tsx(5 hunks)web/src/hooks/useScrollTop.ts(1 hunks)
🔇 Additional comments (4)
web/src/hooks/useScrollTop.ts (1)
8-8: Verify scroll behavior across different browsers
The scroll behavior might vary across different browsers. Consider adding smooth scrolling for better user experience.
- osInstanceRef?.current?.osInstance().elements().viewport.scroll({ top: 0 });
+ osInstanceRef?.current?.osInstance().elements().viewport.scroll({
+ top: 0,
+ behavior: 'smooth'
+ });web/src/components/DisputeView/DisputeInfo/DisputeInfoCard.tsx (3)
4-8: LGTM: Import statements are properly organized
The new imports for Link and useScrollTop are correctly placed and necessary for the new functionality.
65-68: Consider browser compatibility for text-wrap property
The text-wrap: auto property might not be supported in all browsers. Consider adding a fallback or using word-break: break-word for broader compatibility.
106-108: Verify scroll behavior on navigation
The implementation of scrollTop on link click aligns with the PR objectives. However:
- Ensure this doesn't interfere with the browser's default scroll restoration on navigation
- Consider adding a smooth scroll behavior for better UX
You might want to update the scroll behavior:
- onClick={() => scrollTop()}
+ onClick={() => scrollTop({ behavior: 'smooth' })}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
web/src/pages/Home/TopJurors/JurorCard/JurorTitle.tsx (1)
38-40: Consider error handling for chain configurationWhile the
useMemoimplementation is correct, consider adding error handling in case the chain configuration is undefined.const addressExplorerLink = useMemo(() => { - return `${getChain(DEFAULT_CHAIN)?.blockExplorers?.default.url}/address/${address}`; + const chain = getChain(DEFAULT_CHAIN); + if (!chain?.blockExplorers?.default.url) { + console.warn(`Block explorer URL not found for chain ${DEFAULT_CHAIN}`); + return '#'; + } + return `${chain.blockExplorers.default.url}/address/${address}`; }, [address]);web/src/pages/Cases/CaseDetails/Voting/VotesDetails/AccordionTitle.tsx (3)
44-52: Consider adding transition for smoother hover effects.The styled anchor component implementation looks good, but the hover effect could be smoother.
const StyledA = styled.a` + transition: all 0.2s ease-in-out; :hover { text-decoration: underline; label { cursor: pointer; color: ${({ theme }) => theme.primaryBlue}; } } `;
99-101: Consider adding aria-label for better accessibility.While the link implementation is correct, adding an aria-label would improve accessibility for screen readers.
- <StyledA href={addressExplorerLink} rel="noopener noreferrer" target="_blank"> + <StyledA + href={addressExplorerLink} + rel="noopener noreferrer" + target="_blank" + aria-label={`View juror ${juror} on block explorer`} + >
99-101: Consider adding visual indicator for external link.Since this is an external link that opens in a new tab, it would be helpful to add a visual indicator.
Consider adding a small external link icon after the address to indicate that clicking will open a new tab. This could be implemented using an icon from your existing icon library.
web/src/components/EvidenceCard.tsx (3)
120-131: Enhance accessibility of styled linksConsider adding focus styles and improving link visibility:
const StyledA = styled.a` + text-decoration-thickness: 1px; + &:focus-visible { + outline: 2px solid ${({ theme }) => theme.primaryBlue}; + outline-offset: 2px; + } :hover { text-decoration: underline; + text-decoration-thickness: 1px; p { color: ${({ theme }) => theme.primaryBlue}; }
155-161: Optimize explorer link memoizationConsider extracting the base explorer URL to avoid redundant calculations:
+const useExplorerBaseUrl = () => { + return useMemo(() => { + const chainData = getChain(DEFAULT_CHAIN); + return chainData?.blockExplorers?.default?.url ?? '#'; + }, []); +}; const EvidenceCard: React.FC<IEvidenceCard> = ({ ... }) => { + const baseExplorerUrl = useExplorerBaseUrl(); const addressExplorerLink = useMemo(() => { - return `${getChain(DEFAULT_CHAIN)?.blockExplorers?.default.url}/address/${sender}`; + return `${baseExplorerUrl}/address/${sender}`; }, [sender]); const transactionExplorerLink = useMemo(() => { - return `${getChain(DEFAULT_CHAIN)?.blockExplorers?.default.url}/tx/${transactionHash}`; + return `${baseExplorerUrl}/tx/${transactionHash}`; }, [transactionHash]);
179-191: Enhance semantic HTML for better accessibilityConsider adding semantic HTML elements and ARIA labels:
- <StyledA href={addressExplorerLink} rel="noopener noreferrer" target="_blank"> + <StyledA + href={addressExplorerLink} + rel="noopener noreferrer" + target="_blank" + aria-label={`View address ${sender} on block explorer`} + > <p>{shortenAddress(sender)}</p> </StyledA> </AccountContainer> - <StyledA href={transactionExplorerLink} rel="noopener noreferrer" target="_blank"> - <Timestamp>{formatDate(Number(timestamp), true)}</Timestamp> + <StyledA + href={transactionExplorerLink} + rel="noopener noreferrer" + target="_blank" + aria-label={`View transaction from ${formatDate(Number(timestamp), true)} on block explorer`} + > + <Timestamp> + <time dateTime={new Date(Number(timestamp)).toISOString()}> + {formatDate(Number(timestamp), true)} + </time> + </Timestamp> </StyledA>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/src/components/EvidenceCard.tsx(5 hunks)web/src/pages/Cases/CaseDetails/Voting/VotesDetails/AccordionTitle.tsx(4 hunks)web/src/pages/Home/TopJurors/JurorCard/JurorTitle.tsx(2 hunks)
🔇 Additional comments (7)
web/src/pages/Home/TopJurors/JurorCard/JurorTitle.tsx (3)
1-1: LGTM: Proper imports for optimization and chain utilities
The addition of useMemo and chain utilities is appropriate for optimizing the explorer link generation.
Also applies to: 5-5
23-31: LGTM: Well-implemented styled component for link hover effects
The StyledA component properly implements hover effects and maintains consistency with the theme system. The cursor and color changes provide good visual feedback for interactive elements.
45-47: LGTM: Secure external link implementation
The link implementation correctly includes security attributes (rel="noopener noreferrer") and opens in a new tab. This aligns with the PR objective of making addresses clickable.
web/src/pages/Cases/CaseDetails/Voting/VotesDetails/AccordionTitle.tsx (2)
1-1: LGTM! Imports are properly organized.
The addition of useMemo and chain-related imports aligns well with the new functionality for creating clickable address links.
Also applies to: 7-7
91-93: LGTM! Proper memoization of explorer link.
The useMemo hook is correctly used to optimize the address explorer link generation, preventing unnecessary recalculations when other props change.
web/src/components/EvidenceCard.tsx (2)
1-1: LGTM: Import changes are appropriate
The new imports for useMemo and chain-related utilities are necessary for the added functionality.
Also applies to: 12-12
145-154: Previous interface comment still applies
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/src/components/EvidenceCard.tsx (2)
120-131: Consider enhancing accessibility of styled linksThe hover styles are good, but consider adding focus styles for keyboard navigation and ensuring sufficient color contrast for accessibility.
const StyledA = styled.a` + &:focus { + outline: 2px solid ${({ theme }) => theme.primaryBlue}; + outline-offset: 2px; + } :hover { text-decoration: underline; p { color: ${({ theme }) => theme.primaryBlue}; } label { cursor: pointer; color: ${({ theme }) => theme.primaryBlue}; } } `;
180-186: Enhance security for external linksWhile
rel="noopener noreferrer"is correctly used, consider adding additional security attributes for external links.-<StyledA href={addressExplorerLink} rel="noopener noreferrer" target="_blank"> +<StyledA href={addressExplorerLink} rel="noopener noreferrer nofollow" target="_blank"> -<StyledA href={transactionExplorerLink} rel="noopener noreferrer" target="_blank"> +<StyledA href={transactionExplorerLink} rel="noopener noreferrer nofollow" target="_blank">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
subgraph/core/schema.graphql(2 hunks)subgraph/package.json(1 hunks)web/src/components/EvidenceCard.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- subgraph/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- subgraph/core/schema.graphql
🔇 Additional comments (1)
web/src/components/EvidenceCard.tsx (1)
Line range hint 146-195: Implementation successfully meets PR objectives
The changes effectively enhance the UX by:
- Making addresses clickable and linking to blockchain explorers
- Adding transaction hash links
- Improving the overall navigation and interaction design
The implementation is solid with the suggested improvements for error handling and security.
kemuru
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/src/layout/Header/navbar/DappList.tsx (1)
128-130: Consider reordering the POH entry for better UX.Currently, the POH V2 entry is placed between "Kleros Scout" and "Governor". Consider moving it to be grouped with other V2 services at the beginning of the array for better organization and user experience.
const ITEMS = [ { text: "Court V2", Icon: Court, url: "https://v2.kleros.builders/", }, { text: "Curate V2", Icon: Curate, url: "https://curate-v2.netlify.app/", }, { text: "Resolver V2", Icon: Resolver, url: "https://v2.kleros.builders/#/resolver", }, { text: "Escrow V2", Icon: Escrow, url: "https://escrow-v2.kleros.builders/", }, + { + text: "POH V2", + Icon: POH, + url: "https://v2.proofofhumanity.id", + }, { text: "Court V1", Icon: Court, url: "https://court.kleros.io/", }, // ... other V1 entries ... { text: "Vea", Icon: Vea, url: "https://veascan.io", }, { text: "Kleros Scout", Icon: Curate, url: "https://klerosscout.eth.limo", }, - { - text: "POH V2", - Icon: POH, - url: "https://v2.proofofhumanity.id", - }, { text: "Governor", Icon: Governor, url: "https://governor.kleros.io", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/layout/Header/navbar/DappList.tsx(1 hunks)
🔇 Additional comments (1)
web/src/layout/Header/navbar/DappList.tsx (1)
128-130: LGTM! POH V2 update looks good.
The update to POH V2 and its corresponding URL is consistent with the versioning pattern used for other Kleros services.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
web/src/components/DisputeView/DisputeInfo/DisputeInfoCard.tsx (3)
65-68: Consider using more widely supported CSS properties for text wrapping.While
text-wrap: autoworks, it has limited browser support. Consider using more widely supported alternatives:label { &.value { margin-left: 8px; overflow: hidden; text-overflow: ellipsis; - text-wrap: auto; + word-wrap: break-word; + overflow-wrap: break-word; } }
70-80: LGTM: Well-structured styled link component with good hover effects.The styling provides clear visual feedback for interactive elements. Consider adding focus styles for keyboard navigation:
const StyledLink = styled(Link)` width: 100%; - :hover { + :hover, :focus { label { &.value { cursor: pointer; color: ${({ theme }) => theme.primaryBlue}; text-decoration: underline; + outline: none; } } } `;
94-94: LGTM: Good implementation of navigation with scroll-to-top behavior.The implementation improves UX by providing clear navigation and scroll behavior. Consider memoizing the onClick handler:
const DisputeInfoCard: React.FC<IDisputeInfoCard> = ({...}) => { const scrollTop = useScrollTop(); + const handleClick = useCallback(() => scrollTop(), [scrollTop]); ... return ( ... - <StyledLink to={`/courts/${courtId}`} onClick={() => scrollTop()}> + <StyledLink to={`/courts/${courtId}`} onClick={handleClick}> ... ); };Also applies to: 107-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
web/src/assets/svgs/icons/calendar.svgis excluded by!**/*.svgweb/src/assets/svgs/icons/law-balance.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
web/src/components/DisputeView/DisputeInfo/DisputeInfoCard.tsx(5 hunks)
🔇 Additional comments (2)
web/src/components/DisputeView/DisputeInfo/DisputeInfoCard.tsx (2)
4-8: LGTM: New imports are appropriate for the added functionality.
The added imports for Link and useScrollTop align well with the PR objectives of improving navigation and UX.
18-18: LGTM: Type imports are well-structured.
The import of FieldItem and IDisputeInfo types from the index file follows good TypeScript practices.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/src/components/DisputeView/DisputeInfo/DisputeInfoCard.tsx(3 hunks)web/src/components/Field.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/DisputeView/DisputeInfo/DisputeInfoCard.tsx
🔇 Additional comments (2)
web/src/components/Field.tsx (2)
3-3: LGTM! Clean import organization.
The imports are well-organized and the new useScrollTop hook is properly imported.
Also applies to: 7-7
99-100: Verify scroll behavior with navigation.
The scroll functionality implementation needs verification:
- The combination of
scrollTop()with link navigation might create a jarring user experience - Consider adding error handling for the scrollTop function
Let's verify the usage of useScrollTop hook:
Also applies to: 111-111
✅ Verification successful
Let me gather more information about the scroll behavior implementation.
Scroll behavior implementation is correct and follows established patterns
The scroll functionality is properly implemented:
- The
useScrollTophook usesOverlayScrollContextwhich is the app's custom scroll management solution usingoverlayscrollbars - The
scrollTop()withevent.stopPropagation()is the correct pattern as seen in other navigation components - Error handling is not needed as the hook safely handles undefined instances with optional chaining
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of useScrollTop to understand the common patterns
rg -l "useScrollTop"
# Search for scroll-related event handlers in link components
ast-grep --pattern 'onClick={$_}'
Length of output: 115
Script:
#!/bin/bash
# Check the implementation of useScrollTop hook
cat web/src/hooks/useScrollTop.ts
# Look for the complete Field component implementation to understand the context
cat web/src/components/Field.tsx
# Search for any scroll-related handlers with context
rg "scroll" -A 3 -B 3
Length of output: 14880
kemuru
dismissed stale reviews from coderabbitai[bot] and Harman-singh-waraich
via
fd6a186
|
Code Climate has analyzed commit fd6a186 and detected 4 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
web/src/pages/Cases/index.tsx (1)
24-25: LGTM! Routes follow RESTful conventions.The route structure is well-organized:
/:id/evidence/attachment/*for evidence attachments/:id/overview/policy/attachment/*for policy attachmentsBoth routes reuse the same
AttachmentDisplaycomponent, promoting code reuse.Consider adding route constants in a separate file for better maintainability if you have many routes that need to be referenced elsewhere in the application.
web/src/pages/Cases/AttachmentDisplay/index.tsx (2)
Line range hint
52-54: Add aria-label for better accessibilityConsider adding an aria-label to the "Open in new tab" link to improve screen reader experience.
- <NewTabInfo href={url} rel="noreferrer" target="_blank"> + <NewTabInfo + href={url} + rel="noreferrer" + target="_blank" + aria-label="Open attachment in new tab" + > Open in new tab <StyledNewTabIcon /> </NewTabInfo>
Based on the search results, I can now provide a final response regarding the URL sanitization concern:
URL handling needs additional security measures
The current implementation directly uses the URL query parameter without proper sanitization. While there is some URL handling for IPFS URLs through
getIpfsUrl, the main attachment display component lacks proper URL validation and sanitization:
- The URL is directly used from query parameters without validation:
const url = searchParams.get("url")- The URL is passed directly to both the anchor tag and FileViewer component without sanitization
- No URL validation or sanitization functions are present in the codebase for general URLs
Recommendations:
- Add URL validation to ensure only allowed protocols (e.g., http, https, ipfs) are permitted
- Implement URL sanitization to prevent XSS and other injection attacks
- Consider using a URL sanitization library or implementing a whitelist-based validation approach
- Add URL validation before passing to both the anchor tag and FileViewer component
🔗 Analysis chain
Line range hint
40-66: Verify URL sanitizationThe component directly uses the URL from query parameters without sanitization. While React's built-in XSS protection helps, it's important to ensure the URL is properly validated before use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for URL validation or sanitization in the codebase # Look for URL validation utilities or similar URL handling patterns # Search for URL validation utilities rg -A 3 "validateURL|sanitizeURL|isValidURL" # Search for URL-related security measures ast-grep --pattern 'function $_(url: string) { $$$ }'Length of output: 104
Script:
#!/bin/bash # Let's search more broadly for URL-related patterns and security measures # Search for URL handling patterns in the FileViewer component and related files rg -l "FileViewer" | xargs rg -A 5 "url.*=" # Look for any security-related utilities or hooks rg -l "security|sanitize|validate|safe" | xargs rg -A 5 "function|const" # Check if there's any URL validation in the component's dependencies fd FileViewer --type f | xargs rg -A 5 "url" # Search for any URL prop type definitions or interfaces ast-grep --pattern 'interface $_ { url: $_ }'Length of output: 133767
Script:
#!/bin/bash # Let's search for FileViewer component implementation and any URL handling patterns # Look for FileViewer component implementation rg -A 10 "const FileViewer.*=" # Search for any URL validation or sanitization patterns in the component's dependencies rg -A 5 "url.*=|validateUrl|sanitizeUrl|isValid.*url" # Check for any URL-related security measures in the component's dependencies ast-grep --pattern 'const $_ = ($url: string) => { $$$ }' # Look for URL prop type definitions ast-grep --pattern 'type $_ = { url: $_ }'Length of output: 154752
Script:
#!/bin/bash # Let's search for URL validation and sanitization patterns more broadly # Look for URL validation/sanitization functions rg -A 5 "validateUrl|sanitizeUrl|isValid|sanitize|clean.*url" --type ts # Look for URL-related utility functions ast-grep --pattern 'function $_(url: $_) { $$$ }' # Look for URL-related hooks/utils rg -A 5 "useUrl|getUrl|formatUrl|parseUrl" --type ts # Look for URL-related constants/configs rg "URL_PATTERN|URL_REGEX|urlPattern" --type tsLength of output: 8522
Script:
#!/bin/bash # Let's check for URL handling in the FileViewer component and related files # Look for URL handling in FileViewer component rg -A 10 "const FileViewer" --type ts # Look for URL-related utility functions in utils directory rg -A 5 "url" web/src/utils/ # Look for getIpfsUrl usage rg -A 5 "getIpfsUrl" --type ts # Look for any URL-related hooks or context rg -A 5 "useUrl|getUrl" web/src/hooks/Length of output: 7355
web/src/pages/Cases/AttachmentDisplay/Header.tsx (2)
61-67: Consider adding error boundary for navigationThe navigation logic is well-organized and follows React Router best practices. However, consider adding error handling for cases where the history stack is empty.
const handleReturn = () => { - navigate(-1); + try { + navigate(-1); + } catch (error) { + // Fallback to a default route if history is empty + navigate('/cases'); + } };
74-74: Enhance button accessibilityConsider adding ARIA labels and keyboard navigation improvements to the return button.
<StyledButton text="Return" Icon={Arrow} onClick={handleReturn} + aria-label="Return to previous page" + onKeyDown={(e) => e.key === 'Enter' && handleReturn()} />Also applies to: 76-76
web/src/components/DisputePreview/Policies.tsx (1)
67-70: Consider adding hover and focus states for better accessibility.While the styled link implementation is good, consider adding hover and focus states for better accessibility and user interaction feedback.
const StyledLink = styled(Link)` display: flex; gap: 4px; + &:hover, &:focus { + text-decoration: underline; + outline: none; + } + &:focus-visible { + outline: 2px solid ${({ theme }) => theme.primaryBlue}; + outline-offset: 2px; + } `;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/src/components/DisputePreview/Policies.tsx(3 hunks)web/src/pages/Cases/AttachmentDisplay/Header.tsx(2 hunks)web/src/pages/Cases/AttachmentDisplay/index.tsx(2 hunks)web/src/pages/Cases/index.tsx(2 hunks)
🔇 Additional comments (5)
web/src/pages/Cases/index.tsx (1)
8-8: Verify the component renaming impact.
The renaming from EvidenceAttachmentDisplay to AttachmentDisplay suggests a more generic implementation to handle both evidence and policy attachments.
Let's verify the component usage across the codebase:
✅ Verification successful
Component rename is safe and properly implemented
The verification shows that:
- The component has been cleanly renamed with no lingering references to
EvidenceAttachmentDisplay AttachmentDisplayis a simple URL-based component that can handle both evidence and policy attachments- The component is properly used in two routes with consistent patterns
- No other parts of the codebase are affected by this rename
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old component name
# and verify the new component's implementation
# Check for any remaining references to EvidenceAttachmentDisplay
rg "EvidenceAttachmentDisplay"
# Verify AttachmentDisplay implementation handles both evidence and policy cases
ast-grep --pattern 'export default function AttachmentDisplay() {
$$$
}'
Length of output: 107
Script:
#!/bin/bash
# Let's try a different approach to verify the component renaming
# 1. Find the AttachmentDisplay component file
fd "AttachmentDisplay.tsx" --type f
# 2. Look for any git history of the rename
git log -S "EvidenceAttachmentDisplay" --oneline
# 3. Search for any remaining references to either name with more context
rg -A 3 "AttachmentDisplay|EvidenceAttachmentDisplay"
# 4. Check the component's implementation
fd "AttachmentDisplay.tsx" --type f --exec cat {}
Length of output: 1218
web/src/pages/Cases/AttachmentDisplay/index.tsx (1)
40-40: LGTM! Component rename improves reusability
The rename from EvidenceAttachmentDisplay to AttachmentDisplay makes the component more generic and reusable for different types of attachments, aligning well with the new routing structure that handles both evidence and policy attachments.
Also applies to: 67-67
web/src/pages/Cases/AttachmentDisplay/Header.tsx (1)
4-4: LGTM: Import changes are appropriate
The addition of useLocation and useParams hooks is necessary for the new routing functionality.
web/src/components/DisputePreview/Policies.tsx (2)
4-4: LGTM!
The addition of the Link import from react-router-dom is appropriate for client-side routing.
64-64: LGTM!
The addition of align-items: center improves the vertical alignment of items within the container.
PR-Codex overview
This PR focuses on updating the evidence handling and display functionality, enhancing the user interface, and improving code organization. Key updates include version changes, the addition of
transactionHash, and adjustments to component names and imports.Detailed summary
transactionHashto the evidence structure.DappListfrom "POH V1" to "POH V2" and changed its URL.EvidenceAttachmentDisplaytoAttachmentDisplay.AttachmentDisplay.DisputeInfoCardwith enhanced link handling.useScrollTophook for scrolling functionality.EvidenceCardto include transaction links.Summary by CodeRabbit
Summary by CodeRabbit
New Features
transactionHashfield to theEvidenceandClassicEvidenceentities, enhancing evidence tracking.EvidenceCardcomponent to display links to blockchain explorers for the sender's address and transaction.useScrollTophook for improved navigation within the application.DisputeInfoCardcomponent to allow navigation to court details.AccordionTitleandJurorTitlecomponents with links to juror addresses on blockchain explorers.EvidenceAttachmentDisplaycomponent toAttachmentDisplayand added a new route for policy attachments.Bug Fixes
Chores
package.jsonfile to reflect minor improvements.