Skip to content

Conversation

colin-ho
Copy link
Contributor

@colin-ho colin-ho commented Aug 8, 2025

Changes Made

This PR makes 2 changes to the interactive html.

  • Skips the server check at rendering time, as sometimes the server starts after the html is rendered. It's ok if the server is actually not available though, as there is a fallback during onclick to just show the existing truncated data.
  • Fit images to the popup window size even though they are very small.

Related Issues

Checklist

  • Documented in API Docs (if applicable)
  • Documented in User Guide (if applicable)
  • If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
  • Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)

@github-actions github-actions bot added the fix label Aug 8, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR addresses two specific issues with the interactive HTML display functionality in Daft's dashboard system. The changes focus on improving the user experience when viewing data in the browser-based interface.

The first change removes a server availability check that was performed during HTML rendering. Previously, the dashboard would check if the server was running when generating the HTML and would skip attaching event handlers if the server wasn't available. This created a race condition where HTML could be rendered before the dashboard server fully started, permanently disabling interactive functionality even after the server became available. The new implementation always attaches event handlers and relies on existing error handling in fetch requests to gracefully fall back to showing truncated content when the server is unavailable.

The second change standardizes CSS styling for image display in the interactive dashboard. The modifications affect how images are rendered both in truncated mode (within table cells) and in full-size popup mode. For truncated images, the styling was simplified from max-height:128px;width:auto to width:auto;height:auto. For full-size popup images, the styling was changed from max-width:90%;max-height:90vh;width:auto;height:auto to width:100%;height:auto. This ensures that images, even very small ones, properly utilize the available popup window space while maintaining their aspect ratio.

These changes integrate with Daft's broader dashboard infrastructure, which provides interactive data exploration capabilities. The dashboard server (in src/daft-dashboard/src/lib.rs) generates HTML with embedded JavaScript for interactivity, while the image display functionality (in src/daft-image/src/ops.rs) handles the rendering of image data within the dashboard's table view.

Confidence score: 5/5

  • This PR is safe to merge with minimal risk as it addresses specific UI/UX issues without affecting core data processing logic
  • Score reflects straightforward CSS styling changes and removal of problematic conditional logic that was causing race conditions
  • No files require special attention as both changes are well-isolated improvements to existing functionality

2 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

Copy link

codecov bot commented Aug 8, 2025

Codecov Report

❌ Patch coverage is 88.88889% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.89%. Comparing base (bda006a) to head (081821e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-image/src/ops.rs 0.00% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4943      +/-   ##
==========================================
- Coverage   79.27%   78.89%   -0.39%     
==========================================
  Files         908      908              
  Lines      125867   126784     +917     
==========================================
+ Hits        99777   100020     +243     
- Misses      26090    26764     +674     
Files with missing lines Coverage Δ
src/daft-dashboard/src/lib.rs 68.00% <100.00%> (-1.31%) ⬇️
src/daft-image/src/ops.rs 82.23% <0.00%> (-0.64%) ⬇️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@colin-ho colin-ho merged commit b5cb9cd into main Aug 8, 2025
50 checks passed
@colin-ho colin-ho deleted the colin/fix-interactive-html branch August 8, 2025 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant