Add CID to CSV export #1938
Add CID to CSV export #1938
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1938 +/- ##
============================================
- Coverage 16.00% 15.99% -0.01%
Complexity 482 482
============================================
Files 257 257
Lines 7837 7838 +1
Branches 807 807
============================================
Hits 1254 1254
- Misses 6533 6534 +1
Partials 50 50 ☔ View full report in Codecov by Sentry. |
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the CSV export functionality to include a new "cid" column in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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 (7)
src/main/java/ai/elimu/web/content/multimedia/image/ImageCsvExportController.java (1)
Line range hint
46-65: Consider adding documentation and error handling for IPFS migrationSince this is part of the IPFS migration, consider these improvements:
- Add Javadoc explaining the CID field's purpose and its relation to IPFS
- Consider adding logging for cases where CID is null to help track migration progress
Here's a suggested improvement:
"content_license", "attribution_url", "title", + // CID (Content Identifier) for IPFS-stored images "cid", "download_url", "image_format" ); StringWriter stringWriter = new StringWriter(); CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); for (Image image : images) { String downloadUrl = "/image/" + image.getId() + "." + image.getImageFormat().toString().toLowerCase(); + // Log if CID is missing to track IPFS migration progress + if (image.getCid() == null) { + logger.debug("Image ID {} has no CID yet", image.getId()); + } csvPrinter.printRecord(src/main/webapp/WEB-INF/jsp/content/storybook/create.jsp (2)
59-59: Consider adding error handling for IPFS-related issues.As part of the migration to IPFS, we should handle cases where the image URL might be unavailable due to IPFS connectivity issues or missing Pinata credentials.
Consider wrapping the image in a conditional block with appropriate error handling:
<c:if test="${not empty storyBook.coverImage}"> + <c:choose> + <c:when test="${not empty storyBook.coverImage.url}"> <img src="<spring:url value='${storyBook.coverImage.url}' />" alt="${storyBook.title}" /> + </c:when> + <c:otherwise> + <div class="error-message"> + <fmt:message key="image.unavailable" /> + </div> + </c:otherwise> + </c:choose> </c:if>
59-59: Add loading state for better UX.Since IPFS image loading might take longer than traditional database-stored images, consider adding a loading state.
Consider adding loading indication:
- <img src="<spring:url value='${storyBook.coverImage.url}' />" alt="${storyBook.title}" /> + <div class="image-container"> + <div class="preloader-wrapper small active"> + <div class="spinner-layer spinner-blue-only"> + <div class="circle-clipper left"> + <div class="circle"></div> + </div> + </div> + </div> + <img src="<spring:url value='${storyBook.coverImage.url}' />" + alt="${storyBook.title}" + onload="this.parentElement.classList.add('loaded')" /> + </div>src/main/webapp/WEB-INF/jsp/content/storybook/peer-reviews/pending.jsp (1)
Line range hint
42-45: Add CID-related class attribute for consistency.Other JSP files in the codebase (e.g., edit.jsp) include a conditional class based on
coverImage.cid. For consistency, consider adding it here as well.<img src="<spring:url value='${coverImageUrl}' />" + class="${not empty storyBook.coverImage.cid ? 'cid-true' : 'cid-false'}" style="max-width: 64px; border-radius: 8px;"/>src/main/webapp/static/css/styles.css (1)
69-72: Consider enhancing the visual indicator for better accessibility.The orangered border-top effectively highlights images without CID, which is helpful during the database-to-IPFS migration. However, consider these improvements:
.cid-false, main .card-panel img.cid-false { - border-top: 8px solid orangered; + /* Indicate images pending IPFS migration */ + border-top: 8px solid #FF4500; /* Use hex code for consistency */ + outline: 2px solid #FF4500; /* Additional visual indicator for accessibility */ + position: relative; } + +/* Add tooltip to explain the meaning */ +.cid-false::after { + content: "Pending IPFS migration"; + position: absolute; + top: -24px; + left: 0; + background: #FF4500; + color: white; + padding: 2px 6px; + border-radius: 4px; + font-size: 12px; + opacity: 0; + transition: opacity 0.3s; +} + +.cid-false:hover::after { + opacity: 1; +}These changes would:
- Use hex color code for consistency
- Add an outline for better visibility
- Include a tooltip to explain the meaning of the border
- Improve the user experience during the migration phase
src/main/webapp/WEB-INF/jsp/content/storybook/list.jsp (1)
37-37: Consider extracting the repeated code into a JSP fragment.The same code pattern is repeated across five sections. Consider extracting it into a reusable JSP fragment to improve maintainability.
Create a new file
storybook-card.jspf:<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %> <%@ taglib prefix="spring" uri="http://www.springframework.org/tags" %> <c:set var="coverImageUrl" value="" /> <c:if test="${not empty storyBook.coverImage}"> <c:set var="coverImageUrl" value="${storyBook.coverImage.url}" /> </c:if> <a href="<spring:url value='/content/storybook/edit/${storyBook.id}' />"> <div class="card-image cid-${not empty storyBook.coverImage and storyBook.coverImage.cid != null}" style="background-image: url(<spring:url value='${coverImageUrl}' />); background-color: #DDD;"> <span class="card-title"><c:out value="${storyBook.title}" /></span> </div> </a>Then include it in each section:
-<c:set var="coverImageUrl" value="" /> -<c:if test="${not empty storyBook.coverImage}"> - <c:set var="coverImageUrl" value="${storyBook.coverImage.url}" /> -</c:if> -<a href="<spring:url value='/content/storybook/edit/${storyBook.id}' />"> - <div class="card-image cid-${storyBook.coverImage.cid != null}" - style="background-image: url(<spring:url value='${coverImageUrl}' />); background-color: #DDD;"> - <span class="card-title"><c:out value="${storyBook.title}" /></span> - </div> -</a> +<%@ include file="storybook-card.jspf" %>Also applies to: 40-40, 85-85, 88-88, 133-133, 136-136, 181-181, 184-184, 227-227, 230-230
src/main/webapp/WEB-INF/jsp/content/multimedia/image/edit.jsp (1)
18-20: LGTM with suggestions for enhancement.The implementation correctly adds the CID-based class for styling. However, consider these improvements:
- Add loading state indication:
<img class="cid-${image.cid != null}" src="<spring:url value='${image.url}' />" - alt="${image.title}" /> + alt="${image.title}" + loading="lazy" + onerror="this.classList.add('image-load-error')" />
- Add fallback handling:
+<c:choose> + <c:when test="${not empty image.url}"> <img class="cid-${image.cid != null}" src="<spring:url value='${image.url}' />" alt="${image.title}" loading="lazy" onerror="this.classList.add('image-load-error')" /> + </c:when> + <c:otherwise> + <div class="image-placeholder"> + <i class="material-icons">image</i> + <p>Image not available</p> + </div> + </c:otherwise> +</c:choose>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/main/java/ai/elimu/web/content/multimedia/image/ImageCsvExportController.java(2 hunks)src/main/webapp/WEB-INF/jsp/content/multimedia/image/edit.jsp(1 hunks)src/main/webapp/WEB-INF/jsp/content/storybook/create.jsp(1 hunks)src/main/webapp/WEB-INF/jsp/content/storybook/edit.jsp(2 hunks)src/main/webapp/WEB-INF/jsp/content/storybook/list.jsp(5 hunks)src/main/webapp/WEB-INF/jsp/content/storybook/peer-reviews/pending.jsp(1 hunks)src/main/webapp/static/css/styles.css(1 hunks)
🔇 Additional comments (7)
src/main/java/ai/elimu/web/content/multimedia/image/ImageCsvExportController.java (2)
47-47: LGTM: CID header addition is well-placed
The addition of the "cid" column in the CSV header is properly implemented and logically positioned among other image metadata fields.
63-63: Verify CID null handling during export
While the CID field addition is correct, we should ensure proper handling of null CID values for images not yet migrated to IPFS.
Let's verify the Image class implementation and current data state:
✅ Verification successful
Based on the shell script results, I can see that:
- The
Imageclass has proper CID field implementation with standard getter/setter methods - The
ImageEditControlleralready handles null CID values by checkingif (image.getCid() == null)and then generating/setting the IPFS hash - All CID assignments are done through proper setter methods in various controllers
- No TODO/FIXME comments related to CID handling were found
No issues with CID handling in CSV export
The CID field is properly handled in the codebase. The ImageEditController ensures CID values are populated before image updates, and there's a proper null check implementation. The CSV export can safely use image.getCid() as images will have CID values set during creation/edit operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Image class for CID field implementation and current data state
# Check Image class implementation
echo "Checking Image class implementation..."
ast-grep --pattern 'class Image {
$$$
private String cid;
$$$
}'
# Check for potential null handling in Image class
echo "Checking for null handling..."
rg -A 5 "getCid|setCid"
# Check if there are any TODOs or FIXMEs related to CID
echo "Checking for TODO/FIXME comments related to CID..."
rg -i "todo|fixme|xxx|hack|bug.*cid"
Length of output: 15947
src/main/webapp/WEB-INF/jsp/content/storybook/peer-reviews/pending.jsp (2)
38-38: LGTM! The URL construction has been simplified.
The change to directly use storyBook.coverImage.url aligns with the initiative to support IPFS-stored images.
Line range hint 42-45: Verify CID-related class usage across JSP files.
Let's confirm that other JSP files are using the CID-related class attributes as mentioned in the AI summary.
✅ Verification successful
CID-related class attributes are consistently used across JSP files
The search results confirm that CID-related class attributes are consistently used across multiple JSP files in the codebase:
- The pattern
cid-${*.cid != null}is used consistently for images and card elements - The same CID-related class pattern is used in various contexts including storybooks, cover images, and general images
- The implementation is uniform across different views (list views, edit views, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CID-related class attributes in other JSP files
# Expected: Find similar patterns in other JSP files to confirm our suggestion
# Search for CID-related class attributes in JSP files
echo "Searching for CID-related class patterns in JSP files:"
rg -U "class=.*cid-(true|false)" --type jsp
# Search for coverImage.cid usage in JSP files
echo -e "\nSearching for coverImage.cid usage:"
rg "coverImage\.cid" --type jsp
Length of output: 341
Script:
#!/bin/bash
# Let's try a different approach without relying on file type detection
# First, find all JSP files
echo "Finding all JSP files:"
fd ".*\.jsp$"
# Then search for CID-related patterns in those files
echo -e "\nSearching for CID-related class patterns:"
fd ".*\.jsp$" --exec rg -U "class=.*cid-(true|false)" {} || true
echo -e "\nSearching for coverImage.cid usage:"
fd ".*\.jsp$" --exec rg "coverImage\.cid" {} || true
# Also search for general CID-related patterns
echo -e "\nSearching for general CID patterns:"
fd ".*\.jsp$" --exec rg "\.cid" {} || true
Length of output: 7439
src/main/webapp/WEB-INF/jsp/content/storybook/list.jsp (2)
37-37: LGTM! The URL assignment is simplified and CID support is added.
The changes align well with the transition to IPFS storage:
- Direct URL assignment using
storyBook.coverImage.url - Addition of CID-based class for styling
Also applies to: 40-40
37-37: 🛠️ Refactor suggestion
Verify potential null pointer and CSS class usage.
While the changes look good, there are two points to verify:
- The
storyBook.coverImage.cidaccess might throw NPE ifcoverImageis null - The CSS class
cid-${storyBook.coverImage.cid != null}needs corresponding styles
Let's verify these concerns:
Consider adding a null check:
-<div class="card-image cid-${storyBook.coverImage.cid != null}"
+<div class="card-image cid-${not empty storyBook.coverImage and storyBook.coverImage.cid != null}"Also applies to: 40-40, 85-85, 88-88, 133-133, 136-136, 181-181, 184-184, 227-227, 230-230
✅ Verification successful
No NPE risk, CSS class is properly defined and used
Based on the verification results:
-
NPE safety is ensured:
- All JSP files properly check for null
coverImageusing<c:if test="${not empty storyBook.coverImage}">before accessing its properties - The
coverImageUrlis initialized with empty string or placeholder before conditional usage
- All JSP files properly check for null
-
CSS class is properly defined:
- The
cid-falseclass is defined instyles.csswith appropriate styling (orange border) - The class is consistently used across templates with the correct boolean expression
- The
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CSS class definition and potential NPE handling
# Test 1: Check if the CSS class is defined
echo "Checking for CSS class definition..."
rg -l "cid-(true|false)" --type css
# Test 2: Check for null checks in other JSP files
echo "Checking for null handling patterns in other JSP files..."
rg "coverImage\.cid" --type jsp -B 2 -A 2
Length of output: 355
Script:
#!/bin/bash
# Let's check the content of the CSS file and JSP files with better patterns
# Check CSS file content
echo "Checking CSS file content..."
cat src/main/webapp/static/css/styles.css | grep -A 2 -B 2 "cid-"
# Check JSP files with broader pattern for coverImage handling
echo "Checking JSP files for coverImage handling..."
rg "coverImage" --type-add 'jsp:*.jsp' --type jsp -B 2 -A 2
# Check for null checks in JSP files
echo "Checking for null handling patterns..."
rg "empty.*coverImage" --type-add 'jsp:*.jsp' --type jsp -B 2 -A 2
Length of output: 17133
src/main/webapp/WEB-INF/jsp/content/multimedia/image/edit.jsp (1)
18-20: Verify integration with CSS and error handling patterns.
Let's ensure the new CID-based class is properly integrated with the CSS and follows existing error handling patterns.
✅ Verification successful
CSS class integration is properly implemented and follows existing patterns
The verification shows that:
- The CSS class
cid-${image.cid != null}is properly defined instyles.csswith appropriate styling (orange border forcid-false) - The same class pattern is consistently used in other image implementations within the codebase, particularly in
storyBookrelated components - The application has a consistent fallback pattern using placeholder images and proper URL handling through
spring:url
The integration follows the established patterns in the codebase and is correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CSS classes and error handling patterns
# Check for CSS class definitions
echo "Checking for CSS class definitions..."
rg -l "cid-(true|false)" --type css
# Check for similar image error handling patterns
echo "Checking for similar image error handling patterns..."
rg -l "onerror.*image" --type jsp
# Check for material-icons usage in image placeholders
echo "Checking for material-icons usage with images..."
rg "material-icons.*image" --type jsp
Length of output: 516
Script:
#!/bin/bash
# Let's check the CSS file content and JSP files with different approaches
# Check the CSS file content for cid-related classes
echo "Checking CSS file content:"
cat src/main/webapp/static/css/styles.css | grep -A 5 "cid-"
# Check JSP files using different patterns
echo -e "\nChecking JSP files for image handling patterns:"
find src/main/webapp/WEB-INF/jsp -type f -name "*.jsp" -exec grep -l "img.*class=\"" {} \;
# Look for error handling in JSP files
echo -e "\nChecking error handling in JSP files:"
find src/main/webapp/WEB-INF/jsp -type f -name "*.jsp" -exec grep -l "onerror=" {} \;
# Check for similar image implementations
echo -e "\nChecking similar image implementations:"
find src/main/webapp/WEB-INF/jsp -type f -name "*.jsp" -exec grep -B 1 -A 1 "img.*src=" {} \;
Length of output: 35830
Issue Number
Purpose
Technical Details
Testing Instructions
Screenshots
Format Checks
Note
Files in PRs are automatically checked for format violations with
mvn spotless:check.If this PR contains files with format violations, run
mvn spotless:applyto fix them.