Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use monaco for dataclip viewer #2087

Merged
merged 24 commits into from
May 17, 2024
Merged

use monaco for dataclip viewer #2087

merged 24 commits into from
May 17, 2024

Conversation

midigofrank
Copy link
Collaborator

@midigofrank midigofrank commented May 14, 2024

Validation Steps

How can a reviewer validate your work?

Notes for the reviewer

  • Deleted test/lightning_web/live/run_live/streaming_test.exs. It was testing for the scrubbed dataclip lines, so I have moved the test to the test/lightning_web/controllers/dataclip_controller_test.exs.

Checklist

  • The dataclip type is no longer displayed on the step input/output dataclip viewers. I'm not sure how/where to put it
  • Lazy load the monaco editor
  • Configure esbuild to split the monaco editor component bundle correctly?
  • Fix failing tests
  • Do not wait for the whole response body to be loaded in order to display the content. Instead read the body on the fly
  • Only load the editor on visibility
  • Implement an Abort Controller on the JS land???
  • Add HTTP Caching headers. We're using the default browser http cache. The cache is set to expire after 1 day.
  • Some space appears at the top of the viewer if the dataclip is dropped/not saved.
  • Centralize the monaco theme options. It seems like the settings for the Job Editor and the Dataclip Viewer are overriding each other in the Inspector view.

Related issue

Fixes #1872

Review checklist

  • I have performed a self-review of my code
  • I have verified that all appropriate authorization policies (:owner, :admin, :editor, :viewer) have been implemented and tested
  • If needed, I have updated the changelog
  • Product has QA'd this feature

@midigofrank midigofrank self-assigned this May 14, 2024
@midigofrank midigofrank changed the title WIP: use monaco for dataclip viewer use monaco for dataclip viewer May 16, 2024
Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 95.77465% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.08%. Comparing base (a0709fb) to head (09f6dc9).
Report is 4 commits behind head on main.

Current head 09f6dc9 differs from pull request most recent head 2e564a1

Please upload reports for the commit 2e564a1 to get more accurate results.

Files Patch % Lines
lib/lightning/dataclip_scrubber.ex 78.57% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2087      +/-   ##
==========================================
- Coverage   90.09%   90.08%   -0.01%     
==========================================
  Files         254      257       +3     
  Lines        8286     8274      -12     
==========================================
- Hits         7465     7454      -11     
+ Misses        821      820       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGELOG.md Outdated Show resolved Hide resolved
assets/js/dataclip-viewer/component.tsx Outdated Show resolved Hide resolved
@stuartc stuartc marked this pull request as ready for review May 17, 2024 12:20
[ci skip]
@stuartc stuartc merged commit 0458026 into main May 17, 2024
6 checks passed
@stuartc stuartc deleted the improved-dataclip-viewer branch May 17, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Crash loop: performance issues when loading large logs and (input and output) data clips
3 participants