Feature/frontend workbench#89
Conversation
There was a problem hiding this comment.
This PR introduces updates to the frontend workbench by adding new icons, modifying UI layouts, and managing clip visibility and locking mechanisms. It also includes an unexpected change to the backend source_asset_service.py that appears to be an incorrect reassignment of original_gcs_uri.
🔍 General Feedback
- Correctness: The backend change has a high probability of causing a bug or data loss as it sets
original_gcs_urito the newfinal_gcs_uri. Please double check if this is intended. - HTML Integrity: There is an instance of a nested
<button>element which is invalid HTML and can cause click handling to fail. - Code Cleanup: There are some minor spacing issues and unused or empty functions (
hideCurrentVideo) that should be removed to maintain code hygiene. - Redundancies: Simplified some redundant null checks (
!clip) which were repeated right after the initial check.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
This Pull Request introduces significant UI enhancements to the frontend workbench, adding new timeline controls such as clip locking and visibility toggling, alongside a backend logic update for asset uploading. While the logic to manage timeline layers and track overlays is a solid improvement, there are critical syntax errors and logical flaws that must be addressed before this branch can be merged, specifically in the Angular HTML template and TypeScript components.
🔍 General Feedback
- The new feature icons (
visibility-off,lock-open-right) have been cleanly integrated into the frontend assets directory and correctly registered in the workbench component. - The
resolveOverlapsmethod in the workbench component received a major rewrite with an improved "plow/ripple insert" logic that seems well-structured. - Ensure all variable references are carefully checked after performing code deletions or refactoring to prevent
ReferenceErrors. - An empty method (
hideCurrentVideo()) was added in theworkbench.component.tsfile without implementation. Consider either implementing it, adding a// TODOcomment, or removing it entirely to maintain code cleanliness.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
This Pull Request introduces significant UI enhancements to the workbench, including new track locking and visibility toggle features. While the additions improve the application's capabilities, several critical syntax and logic issues were identified that require correction before merging.
🔍 General Feedback
- The introduction of per-clip locking and visibility controls is a great addition to the timeline functionality.
- Several HTML syntax errors were introduced in the component template, including missing closing tags and nested buttons.
- Ensure that variable references are fully updated during refactoring (e.g., removing
idreferences in the TypeScript file). - Keep the codebase clean by removing empty, unused functions before merging.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
The pull request introduces new visual features and lock/visibility controls for the timeline workbench in the frontend, along with an update to asset handling in the backend. While the general approach is well-structured, there are a few critical errors that must be addressed before merging, including an Angular template compilation error, runtime reference errors, and an invalid HTML structure.
🔍 General Feedback
- Frontend Template Integrity: Be cautious when using
@letsyntax in Angular templates. Ensure variables are not re-declared in the same logical scope to avoid build failures. - HTML Validity: Ensure elements like
<button>are not nested within one another, as this violates HTML specifications and breaks accessibility tools. - Variable Cleanup: When refactoring out variables (like
id), always double-check the surrounding conditions to ensure they are fully removed to prevent runtimeReferenceErrors. - Backend Data Integrity: Pay close attention to data assignments to ensure existing tracking fields (like
original_gcs_uri) aren't unintentionally overwritten.
There was a problem hiding this comment.
This PR introduces valuable enhancements to the frontend workbench, including new clip locking and visibility features. The implementation is generally solid, though there are a few styling, formatting, and HTML structure issues that should be addressed.
🔍 General Feedback
- Formatting: Several places in the code have incorrect indentation (e.g., using 4 or 6 spaces instead of 2 in the Angular components) and whitespace around keyword arguments in Python.
- Accessibility/HTML: Be careful with nesting interactive elements like
<button>tags, as this leads to invalid HTML and accessibility concerns. - Redundancy: Watch out for redundant conditional checks, such as checking for a null
clipright after already checking and returning on the same condition.
There was a problem hiding this comment.
This Pull Request introduces significant enhancements to the frontend workbench, adding clip-level locking and visibility controls along with necessary UI updates and SVG icons. However, the review revealed a critical HTML validity issue with nested buttons and a bug in the backend where the original GCS URI was incorrectly overwritten.
🔍 General Feedback
- Constructive Addition: The overall structure of the new timeline clip controls (locking, visibility) is well integrated into the existing layout and component logic.
- Code Consistency: Several newly introduced variables in the template use loose equality (
==) which deviates from standard TypeScript/Angular practices preferring strict equality (===). - Formatting: Minor code formatting inconsistencies, such as extraneous blank lines or slightly altered indentation in
workbench.component.ts, should be cleaned up before merging.
Fixes #<issue_number_goes_here>