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

fix(BrowserStorage): don't generate empty entries in BrowserStorage #11718

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Mar 4, 2024

☑️ Resolves

  • Fix empty entries like 'virtualBackgroundType_' in local storage without defined key
    • this.get('token') sometime returns nothing, as webrtc isn't always initialising it
    • Prevent generating empty entries again by providing a token
    • Add init function to cleanup them while initialising
  • reset virtualBackground at media preview, if none is set for conversation

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏡 After

reset-virtual-bg.webm

🚧 Tasks

  • Code review

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖥️ Tested with Desktop client or should not be risky for it
  • ⛑️ Tests are included or not possible

@Antreesy Antreesy added this to the 💞 Next Major (29) milestone Mar 4, 2024
@Antreesy Antreesy requested a review from ShGKme March 4, 2024 13:25
@Antreesy Antreesy self-assigned this Mar 4, 2024
@ShGKme ShGKme requested a review from danxuliu March 4, 2024 13:30
@ShGKme
Copy link
Contributor

ShGKme commented Mar 4, 2024

  • this.get('token') sometime returns nothing, as webrtc isn't always initialising it

Is it a bug or expected behavior?

@Antreesy
Copy link
Contributor Author

Antreesy commented Mar 4, 2024

Found only one setup here:

_setInitialState(localStream) {
this.set('token', store.getters.getToken())
this._updateMediaAvailability(localStream)
this.set('raisedHand', { state: false, timestamp: Date.now() })
},

should be set change on each setWebRtc (each call), while LocalmediaModel is persistent

@ShGKme
Copy link
Contributor

ShGKme commented Mar 4, 2024

With this PR those methods use 2 tokens - from an argument and from context. It is not clear for me, what is the difference between them, when one should be used and when the other.

If the token from the context is wrong, but it is not supposed to be wrong, we probably should find why it is wrong and fix it.

If it cannot be correct in this case, or we probably should get rid of it at lease in these operations.

But I'd avoid intermediate solution for further support.

src/stores/settings.js Outdated Show resolved Hide resolved
@Antreesy
Copy link
Contributor Author

Antreesy commented Mar 4, 2024

we probably should find why it is wrong and fix it.

Got the context so far: see signaling method join call in 'webrtc/index.js' call stack and code:
image

  • L157: start of the method
    -L167-188: calling public methods of localMediaModel (token is not defined ATM : ⚠️ )
  • L201: webrtc.startMedia returns Promise, upon fullfillment of which _setInitialState is triggered: here is where we set 'token'.

All places that use this.get('token') in LocalMediaModel are responsible only for interacting with BrowserStorage, so should be safe to add solution:

  • L166: set token when it's available

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Nice catch with the undefined token! :-)

Unfortunately I had no time to find the steps to reproduce that issue (although I saw it in the past), but code looks good 👍 A little comment either in the code or in the commit would be nice for future reference.

Independently of that I left some minor comments.

src/init.js Outdated Show resolved Hide resolved
src/stores/settings.js Outdated Show resolved Hide resolved
src/utils/webrtc/index.js Outdated Show resolved Hide resolved
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/10085/cleanup-bg-per-room branch from 285a229 to 4a7b43b Compare March 7, 2024 17:13
@Antreesy Antreesy enabled auto-merge March 7, 2024 17:13
@Antreesy Antreesy disabled auto-merge March 7, 2024 17:50
@Antreesy Antreesy changed the title fix(BrowserStorage): cleanup empty entries in BrowserStorage fix(BrowserStorage): don't generate empty entries in BrowserStorage Mar 7, 2024
@Antreesy Antreesy force-pushed the fix/10085/cleanup-bg-per-room branch from 4a7b43b to 150005c Compare March 7, 2024 18:11
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Cleaner now, thanks for the investigation! 💙

Agree with Daniel's nitpicks.

- set token at localMediaModel on call start
- add check for current token in settings store

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/10085/cleanup-bg-per-room branch from 150005c to e775fb1 Compare March 7, 2024 22:01
@Antreesy Antreesy enabled auto-merge March 7, 2024 22:02
@Antreesy Antreesy merged commit 6bb1bfc into main Mar 7, 2024
45 checks passed
@Antreesy Antreesy deleted the fix/10085/cleanup-bg-per-room branch March 7, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants