Skip to content
This repository was archived by the owner on Apr 18, 2024. It is now read-only.

Conversation

@bmartel
Copy link
Contributor

@bmartel bmartel commented Mar 10, 2023

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Frontend

Describe the reason for change

When requesting urls for audio, it sometimes produces a CORS error after which it doesn't resolve and remains in error until the browser cache is cleared.
There is a problem with when the browser requests Partial Content for media based requests, having a completely different Header response than a full content Get request that is made. This eventually leads to the Partial Content request getting cached in the browser, and when trying to issue a Get request to that same url produces a CORS error.

What does this fix?

This fix just puts a small query param added to only the Get requests made for the waveform data. It is small and deterministic purposefully so we don't ruin cache which would cause many more issues. This allows a differentiation between the Get requests made for waveform data of the audio files and the media requests (which we want to cache naturally for themselves, for both video and audio).

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

Audio v3

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (53d62b1) 65.36% compared to head (323ca76) 65.36%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1234   +/-   ##
=======================================
  Coverage   65.36%   65.36%           
=======================================
  Files         402      402           
  Lines       25198    25200    +2     
  Branches     6449     6450    +1     
=======================================
+ Hits        16470    16473    +3     
+ Misses       8728     8727    -1     
Impacted Files Coverage Δ
src/lib/AudioUltra/Media/MediaLoader.ts 93.75% <100.00%> (+0.13%) ⬆️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bmartel bmartel enabled auto-merge (squash) March 15, 2023 13:44
@bmartel bmartel merged commit d847f70 into master Mar 15, 2023
@bmartel bmartel deleted the fb-lsdv-4593-cors branch March 15, 2023 14:10
nick-skriabin pushed a commit that referenced this pull request Mar 31, 2023
* fix: LSDV-4593: CORS error requesting audio

* use a less likely query param, ensure all relative urls are handled so 404 errors are thrown correctly
nick-skriabin added a commit that referenced this pull request Apr 14, 2023
* Introducing Cypress

* Remove outputs

* Remove old outputs

* Add logs

* fix: LSDV-4676: Region corruption MIG (#1230)

* Fix regions corruption by utilizing existing result

* Cover data export with tests

* Only define _rawResult for newly created areas

* Fix tests + Fix segmentation lookup

* Update src/stores/Annotation/Annotation.js

Co-authored-by: bmartel <brandonmartel@gmail.com>

* Fix lint issue

* Remove excess trace

* Wrap `_rawValue` with feature flag

* Fix incorrect export data + Assertion

---------

Co-authored-by: bmartel <brandonmartel@gmail.com>

* docs: LSDV-4707: Remove Repeater from docs (#1235)

* fix: LSDV-4715: Start, end and offset of paragraph selection should not include empty or newline (#1233)

* fix: LSDV-4715: Start and offset of paragraph selection should not include empty or newline

* fix empty lines

* fix empty lines when selecting across collapsed text

* adding tests for paragraph selection fixes

* Update Paragraphs.js

* adding tests for paragraph selection end offsets

* Add test case for multi-line selection with wrong offsets

* Update Paragraphs.js

* linting

* remove console logs

* changing the implementation to base off indices for clarity

* putting back the comment, mistakenly removed

* Update Paragraphs.js

* Fix offset when we move one phrase back

* Fix test to have phrases with different length

---------

Co-authored-by: Sergey <sergey.koshevarov@heartex.com>
Co-authored-by: hlomzik <hlomzik@gmail.com>

* fix: LSDV-4649: Regrouping regions after switching annotations (#1223)

* fix: LSDV-4649: Regrouping regions after switching annotations

* test: Add plugin to monitoring errors from the browser in e2e

* test: LSDV-4649: Add test for checking drag and drop at regions tree in outliner

* Fix creating paragraph regions according to failed tests

* Add plugin for modifying e2e step logs to filter unnecessary information

* Fix tests

* Fix function naming in logs

* Clear code

* Add comment for test fix

---------

Co-authored-by: bmartel <brandonmartel@gmail.com>

* fix: LSDV-4662: Use empty range GET requests instead of HEAD to avoid CORS issue on video presigned urls (#1225)

fix: LSDV-4662: Use empty range GET requests instead of HEAD to avoid CORS issue

* fix: LSDV-4593: CORS error requesting audio (#1234)

* fix: LSDV-4593: CORS error requesting audio

* use a less likely query param, ensure all relative urls are handled so 404 errors are thrown correctly

* feat: LSDV-3023: Annotation Tab Carousel (#1188)

* feat: dev-3879: Control bar

* lint clean up

* visual tweaks

* control bar visual improvements

* cleanup

* visual tweak for view all button

* visual tweak for auto annotation

* feat: DEV-3879: Control bar (#1075)

* applying changes based on feedback

* adding reset button

* feat: LSDV-3023: Annotation Tab Carousel

* prepping button for usage

* tweaking the icons

* more icon tweaks

* no using mobx observer

* now using mobx observer

* adding some icons + clean up

* annotation state icons added and working on carousel

* carousel working 🎉

* we can now update button capability when window is resized

* resolving an error picked up in e2e

* minor tweak for icon when dropdown is open

* have a working test for AnnotationCarousel and AnnotationButton

* making sure bottombar height is correct when FF is off

* check for pk value before ground truth button display

* cannot duplicate drafts

* sort buttons by pk

* viewall vertical alignment

* side-pannels do not pad when hidden in view all

* tab bar recalculates carosel on add/remove entity

* update use effect with entities.length

* space after set and unset

* check history for that action type and entity for can undo states

* create new button added to top bar

* remove avatar icons

* removing unused icon imports

---------

Co-authored-by: yyassi-heartex <yyassi-heartex@users.noreply.github.com>
Co-authored-by: Travis1282 <travisjosephclark@gmail.com>
Co-authored-by: bmartel <brandonmartel@gmail.com>

* fix: LSDV-4752: Support relations in MIG scenario (#1240)

* Support relations in MIG scenario

* Remove excess import

* Fix relations checks

* Render relation if above checks were skipped

* Fix formatting

* Remove excess checks

* Fix check

* ci: Number of tests is increasing, need more time (#1256)

* fix: LSDV-4740: Video Rectangles are displaying while drawing (#1258)

* docs: LSDV-4754: Update Audio documentation to point to v3 (#1242)

* docs: LSDV-4574: Update Audio documentation to point to v3

* this apparently shouldn't be committed

* Update Paragraphs.js

* Add some comment about return type

JSDoc can't parse tuple type, so let it be just `Array` with comment

* Escape tags in Labels and Choices docs

* "publicly" spelling

---------

Co-authored-by: hlomzik <hlomzik@gmail.com>

* ci: update autolabeler.yml

* fix: LSDV-4801: Better file type playable detection for video (#1259)

* fix: LSDV-4801: Better file type playable detection for video

* adding unit test for VirtualVideo filetype detection

* fixing cache invalidation of urls, to not operate on signed urls as it will invalidate the signature

* fix preview loading

* fix audio resolving in tests

* fix: LSDV-4747: Selecting the end character on a Paragraph phrase to the very start of other phrases only includes the first phrase selection (#1257)

* adding another test case to paragraphs for selection issues

* Look for correct text node when selection ends on div

But range stored in region should also be fixed and not have 0 as the endOffset

* changing the test to select a more easily reproducible case manually

* fix highlighting of spans

* adding test for loading paragraph ranges correctly, still need to get this passing but now closer to a solution

* making test for initializing a paragraph region pass

* for some reason using Dialogue as the default dev config causes timeout errors

* fix smoke test for audio paragraphs

* making the result text function simplified to use existing phraseElements

* removing unused function

* adding another test case, and making a slight adjustment to fix

* updating codept timeout as more tests are pushing the threshold

---------

Co-authored-by: hlomzik <hlomzik@gmail.com>

* fix: LSDV-4811: Impossible to select and drag a hidden region (#1270)

* fix: LSDV-4708: Scroll page to canvas only if needed (#1268)

* fix: LSDV-4708: Scroll page to canvas only if needed

If canvas is already in the viewport, don't scroll the page!

* Scroll only if image region is not visible

scrollIntoViewIfNeeded scolls even if it's not really needed :(

* Add comments; fix zoomed image; add smooth

- hard to calculate region position on zoomed image and it can be hidden,
  so don't scroll if image is zoomed in
- add behavior: smooth for better UX

* Improve scroll to region

* Huge improvement for zoomed and huge images

Zoomed in images makes it hard to calculate region bboxes,
so we are trying to make the whole image visible enough.

* Rename method for better readability

---------

Co-authored-by: yyassi-heartex <104568407+yyassi-heartex@users.noreply.github.com>

* ci: SRE-471: Publish tags from LSF repo to LS (#1276)

* fix: LSDV-4692: Brush segmentation is not supported (#1247)

* Fix tools in MIG

* Remove excess FF

* Make RLE Export stageless

* Remove feature flag

* Fix comment

* Remove unused helper

* Remove unused code

* fix: LSDV-4774: Drawing on the hidden image in MIG scenario (#1254)

Fixing Brush and Polygon can draw on hiddent images

* Fix inability to edit brush regions

* Remove RLE cache

* Fix helper

* Rename test suite

* Remove ESLint debug info

* Fix audio assertions

* Fix audio error matching

---------

Co-authored-by: Sergey <sergey.koshevarov@heartex.com>

* Separating cypress into own repo

* Cypress detached

* Remove output from git

* Enable Cypress in CI

* Rename workflow

* Enable parallel running

* Parallel running

* Change ls-test source

* Setup SSH to install private packages

* Update workflow

* Run SSH agent before setting up

* Manual ssh setup

* Update secret name

* Properly pass secrets to the workflow

* Use SSH action

* Properly init SSH

* Fix variables

* Default feature flags for tests

* Set default feature falgs before every test

* Fix feature flags

* Allow setting feature flags after navigation

* Allow setting feature flags on page load

* Canvas comparison test

* Fix tests

* Rename test suite

* Move test, enable test coverage collection

* Enable code coverage upload

* Fix feature falgs

* Run tests in chrome

* Rename the suite, update coverage toggle

* Change Cypress run CI

* Change coverage dir

* Install specific version of the test plugin

* Wrong package

* Change coverage directory

* Enable testing environment

* Use different version of coverage uploader

* Change coverage uploader name

* Run Cypress in parallel

* Update coverage settings

* Fix coverage preparation command

* Playing with coverage

* Fix coverage command

* Just a commit

* Compare images

* Enable source checkout for coverage upload

* Move cypress tests to tests/functional

* Improve feature flag testing

* Remove unnecessary mapping

* Remove unnecessary feature flags stuff

* Rename external feature flags, update ls-test

* Rename init preventer variable

* Switch CI/CD to master

---------

Co-authored-by: bmartel <brandonmartel@gmail.com>
Co-authored-by: hlomzik <hlomzik@gmail.com>
Co-authored-by: Sergey <sergey.koshevarov@heartex.com>
Co-authored-by: yyassi-heartex <104568407+yyassi-heartex@users.noreply.github.com>
Co-authored-by: yyassi-heartex <yyassi-heartex@users.noreply.github.com>
Co-authored-by: Travis1282 <travisjosephclark@gmail.com>
Co-authored-by: Julio Sgarbi <julio.sgarbi@hotmail.com>
Co-authored-by: Sergey Zhuk <sergey.zhuk@heartex.com>
Co-authored-by: Nikita Belonogov <nikita.belonogov@heartex.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants