feat: render images/video/audio in run modal#218
Merged
JeanKaddour merged 2 commits intomainfrom Mar 14, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 47c0872 in 2 minutes and 59 seconds
More details
- Looked at
75lines of code in1files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. frontend/src/components/modals/RunModal.tsx:188
- Draft comment:
Consider using the URL constructor instead of string concatenation for building file paths, e.g. using new URL('api/files/' + filePath, window.location.origin). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The URL constructor is generally considered a more robust way to build URLs as it handles edge cases like double slashes and proper URL encoding. However, in this case the inputs are very controlled - we're just concatenating window.location.origin with a known path prefix and a file path. The current approach is simple and unlikely to cause issues given the controlled inputs.
The URL constructor would be more robust and handle edge cases better. It's technically a best practice for URL manipulation.
While the URL constructor is generally better, in this specific case with controlled inputs and simple concatenation, the current code is clear and sufficient. The suggested change adds complexity without meaningful benefit.
The comment suggests a technically correct but unnecessary improvement. The current code is simple and sufficient for its controlled use case.
2. frontend/src/components/modals/RunModal.tsx:194
- Draft comment:
Image rendering block looks good; consider extracting the repeated tooltip/icon markup into a helper component for maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. frontend/src/components/modals/RunModal.tsx:214
- Draft comment:
The video rendering block duplicates tooltip and layout logic; extracting common media render logic might improve code reuse and consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. frontend/src/components/modals/RunModal.tsx:232
- Draft comment:
Audio rendering is implemented similarly; consider abstracting common UI elements between image, video, and audio previews to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. frontend/src/components/modals/RunModal.tsx:189
- Draft comment:
Use template literals for API endpoint construction for improved readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. frontend/src/components/modals/RunModal.tsx:194
- Draft comment:
Consider moving the supported file extension arrays (for images, videos, audio) to constants outside the function to avoid recreating them on each render. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. frontend/src/components/modals/RunModal.tsx:184
- Draft comment:
Using simple string splitting to extract the file extension might fail with complex filenames (e.g., with query parameters). Consider a more robust parsing approach. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. frontend/src/components/modals/RunModal.tsx:193
- Draft comment:
The image, video, and audio rendering blocks share a similar structure. Consider extracting these into separate helper components to reduce duplication and improve maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_nmfYBHBeP8duZ6EV
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a107293 in 1 minute and 16 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/components/modals/RunModal.tsx:393
- Draft comment:
Ensure removal of the 'Test Cases' header is intentional. Removing it might affect UI clarity or accessibility. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the PR author to confirm their intention regarding the removal of a UI element, which is not allowed according to the rules. However, it does point out a potential issue with UI clarity or accessibility, which could be a valid concern. The comment could be rephrased to suggest reviewing the impact on UI clarity or accessibility without asking for confirmation.
2. frontend/src/components/modals/RunModal.tsx:393
- Draft comment:
Removed the header "Test Cases". Please confirm this change was intentional as it removes contextual labeling from the table. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm their intention, which violates the rule against asking for confirmation. It does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
Workflow ID: wflow_VMfI5nFXF8ep93uz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces enhancements to the
RunModalcomponent in thefrontend/src/components/modals/RunModal.tsxfile. The primary change involves handling different file types more effectively by converting local file paths to API endpoints and rendering appropriate previews for images, videos, and audio files.Enhancements to file handling and rendering:
test_files/orrun_files/to API endpoints.png,jpg,jpeg,gif,webp,svg).mp4,webm,ogg,ogv,mov).mp3,wav,ogg,m4a).Important
Enhance
RunModalto render images, videos, and audio by converting local file paths to API endpoints.test_files/orrun_files/to API endpoints inRunModal.tsx.png,jpg,jpeg,gif,webp,svg.mp4,webm,ogg,ogv,mov.mp3,wav,ogg,m4a.This description was created by
for a107293. It will automatically update as commits are pushed.