feat: Add collapsible blob list in DA Explorer #3033
Conversation
When submissions contain many blobs (e.g., 14,000+), the page became unusable due to all blob hashes being rendered inline. This change adds a collapsible UI: submissions with >5 blobs show a preview of the first 3 with an expandable button to reveal the full list in a scrollable container. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Claude finished @tac0turtle's task in 1m 46s —— View job Code Review Complete ✅I've reviewed PR #3033 that adds collapsible blob list functionality to the DA Explorer. Overall, this is a well-implemented, focused feature that addresses a real usability issue. The code is clean, follows Go idioms, and the implementation is straightforward. ✅ Strengths
🔍 Issues Found1. JavaScript Bug: Button Text Not Initialized (pkg/rpc/server/templates/da_visualization.html:250)Severity: Medium The btn.dataset.expandText = btn.textContent; // Line 254: stores "+X more"
btn.textContent = 'Show less'; // Line 255: changes to "Show less"
// ... later on collapse:
btn.textContent = btn.dataset.expandText; // Line 250: restores "+X more" ✅However, on the first click to expand, line 250 tries to restore from Fix: Initialize if (isExpanded) {
fullList.classList.remove('expanded');
preview.style.display = 'inline';
btn.textContent = btn.dataset.expandText || btn.textContent; // Add fallback
} else {
fullList.classList.add('expanded');
preview.style.display = 'none';
if (!btn.dataset.expandText) { // Store original text on first expand
btn.dataset.expandText = btn.textContent;
}
btn.textContent = 'Show less';
}2. Template Function Type Safety (pkg/rpc/server/da_visualization.go:518-526)Severity: Low - Enhancement Opportunity The new template functions ( Current: "subtract": func(a, b int) int {
return a - b
},Recommendation: While Go's template engine does some type checking, consider defensive validation for robustness: "subtract": func(a, b int) (int, error) {
return a - b, nil
},Or keep simple functions as-is if template type safety is trusted (acceptable for internal use). 3. Potential XSS Risk (Low) (pkg/rpc/server/templates/da_visualization.html:199)Severity: Low - Already Mitigated The button text uses user-controlled data (
Status: No action needed, but good to note for review. 🎯 Code Quality ObservationsGo Template Functions (pkg/rpc/server/da_visualization.go:518-526)Pros:
Considerations:
HTML/CSS/JavaScript (pkg/rpc/server/templates/da_visualization.html)Pros:
Considerations:
🚀 Performance & Edge CasesPerformance: ✅ Good
Edge Cases Handled: ✅ Mostly Good
📋 Testing RecommendationsWhile this PR doesn't include tests (understandable for UI/visualization code), consider manual testing:
🔒 Security AssessmentOverall: ✅ Secure
🎨 Accessibility Suggestions (Optional Enhancement)For better screen reader support, consider: <button class="blob-toggle" onclick="toggleBlobs(this)" aria-expanded="false">
+{{subtract $numBlobs 3}} more
</button>And update JavaScript: function toggleBlobs(btn) {
const isExpanded = fullList.classList.contains('expanded');
btn.setAttribute('aria-expanded', !isExpanded);
// ... rest of function
}✨ Minor Polish Suggestions
📊 Summary
Recommendation: ✅ Approve with minor fix The JavaScript button text bug should be fixed before merging (see Issue #1 above). Everything else is optional polish. The core functionality is solid and addresses the stated problem effectively. 📝 Files Reviewed
|
Summary of ChangesHello @tac0turtle, 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 significantly enhances the user experience of the Data Availability Explorer by introducing a collapsible display for blob IDs. Previously, pages with numerous blobs would become unresponsive due to excessive rendering. The new implementation addresses this by showing only a limited preview of blob IDs by default, with an interactive button to reveal the complete list, thus optimizing page load times and overall responsiveness. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
![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 collapsible UI for the blob list in the DA Explorer, which is a great improvement for usability when dealing with a large number of blobs. The implementation looks solid. I've added a couple of suggestions to improve the maintainability of the HTML template and the associated JavaScript. Specifically, I've recommended refactoring the JavaScript toggle function for conciseness and using a Go template define block to avoid duplicating the blob link HTML.
| {{if le $numBlobs 5}} | ||
| <div class="blob-ids"> | ||
| {{$namespace := .Namespace}} | ||
| {{range .BlobIDs}} | ||
| <a href="/da/blob?id={{.}}&namespace={{$namespace}}" class="blob-link blob-id" title="Click to view blob details">{{slice . 0 8}}...</a> | ||
| {{end}} | ||
| </div> | ||
| {{else}} | ||
| <div class="blob-ids"> | ||
| <span class="blob-ids-preview"> | ||
| {{range $i, $id := .BlobIDs}}{{if lt $i 3}}<a href="/da/blob?id={{$id}}&namespace={{$namespace}}" class="blob-link blob-id" title="Click to view blob details">{{slice $id 0 8}}...</a>{{end}}{{end}} | ||
| </span> | ||
| <button class="blob-toggle" onclick="toggleBlobs(this)">+{{subtract $numBlobs 3}} more</button> | ||
| <div class="blob-ids-full"> | ||
| {{range .BlobIDs}} | ||
| <a href="/da/blob?id={{.}}&namespace={{$namespace}}" class="blob-link blob-id" title="Click to view blob details">{{slice . 0 8}}...</a> | ||
| {{end}} | ||
| </div> | ||
| </div> | ||
| {{end}} |
There was a problem hiding this comment.
The HTML for rendering a blob link is duplicated in three places within this block (for <=5 blobs, for the preview of >5 blobs, and for the full list of >5 blobs). This could lead to inconsistencies if the link format needs to be changed in the future.
To improve maintainability, you could define a reusable template for the link using {{define}}. For example, you could add this somewhere in your template file (e.g., at the top):
{{define "blobLink"}}<a href="/da/blob?id={{.id}}&namespace={{.namespace}}" class="blob-link blob-id" title="Click to view blob details">{{slice .id 0 8}}...</a>{{end}}Then, you can replace each instance of the <a> tag with a call to this template, for example:
{{range .BlobIDs}}{{template "blobLink" (dict "id" . "namespace" $namespace)}}{{end}}This change would centralize the blob link's HTML, making it easier to update.
| function toggleBlobs(btn) { | ||
| const container = btn.parentElement; | ||
| const fullList = container.querySelector('.blob-ids-full'); | ||
| const preview = container.querySelector('.blob-ids-preview'); | ||
| const isExpanded = fullList.classList.contains('expanded'); | ||
|
|
||
| if (isExpanded) { | ||
| fullList.classList.remove('expanded'); | ||
| preview.style.display = 'inline'; | ||
| btn.textContent = btn.dataset.expandText; | ||
| } else { | ||
| fullList.classList.add('expanded'); | ||
| preview.style.display = 'none'; | ||
| btn.dataset.expandText = btn.textContent; | ||
| btn.textContent = 'Show less'; | ||
| } | ||
| } |
There was a problem hiding this comment.
The toggleBlobs function can be made more concise by using classList.toggle() and simplifying the logic. The current implementation is correct, but a more streamlined version would be easier to read and maintain.
| function toggleBlobs(btn) { | |
| const container = btn.parentElement; | |
| const fullList = container.querySelector('.blob-ids-full'); | |
| const preview = container.querySelector('.blob-ids-preview'); | |
| const isExpanded = fullList.classList.contains('expanded'); | |
| if (isExpanded) { | |
| fullList.classList.remove('expanded'); | |
| preview.style.display = 'inline'; | |
| btn.textContent = btn.dataset.expandText; | |
| } else { | |
| fullList.classList.add('expanded'); | |
| preview.style.display = 'none'; | |
| btn.dataset.expandText = btn.textContent; | |
| btn.textContent = 'Show less'; | |
| } | |
| } | |
| function toggleBlobs(btn) { | |
| const container = btn.parentElement; | |
| const fullList = container.querySelector('.blob-ids-full'); | |
| const preview = container.querySelector('.blob-ids-preview'); | |
| const isNowExpanded = fullList.classList.toggle('expanded'); | |
| preview.style.display = isNowExpanded ? 'none' : 'inline'; | |
| if (isNowExpanded) { | |
| btn.dataset.expandText = btn.textContent; | |
| btn.textContent = 'Show less'; | |
| } else { | |
| btn.textContent = btn.dataset.expandText; | |
| } | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3033 +/- ##
==========================================
- Coverage 55.38% 55.33% -0.06%
==========================================
Files 117 117
Lines 11674 11685 +11
==========================================
Hits 6466 6466
- Misses 4483 4494 +11
Partials 725 725
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When submissions contain many blobs (e.g., 14,000+), the page became unusable due to all blob hashes being rendered inline. This change adds a collapsible UI: submissions with >5 blobs show a preview of the first 3 with an expandable button to reveal the full list in a scrollable container.
Overview