search

invisible-college / tawk.space

Social video chats
https://tawk.space
Apache License 2.0
14 stars 1 forks source link

Crash in presence.coffee #34

Closed toomim closed 6 years ago

toomim commented 6 years ago

I got a crash in tawk.space -- looks like in presence.coffee -- which seems to be freezing the UI so that when a new person joins, you can't see them.

Here's the crash:

Uncaught TypeError: Cannot read property 'getBoundingClientRect' of undefined
  at guess_cursor_location (https://tawk.space/client/presence.coffee:188:26)
  at t.dom.CURSOR (https://tawk.space/client/presence.coffee:130:9)
  at t.render (https://stateb.us/client6.js:13:7083)
  at t.<anonymous> (https://stateb.us/client6.js:13:1196)
  at t [as render] (https://stateb.us/client6.js:1:10671)
  at t.<anonymous> (https://stateb.us/client6.js:3:24328)
  at t.i [as _renderValidatedComponent] (https://stateb.us/client6.js:5:15756)
  at t.<anonymous> (https://stateb.us/client6.js:3:23431)
  at t.i [as updateComponent] (https://stateb.us/client6.js:5:15756)
  at t._performComponentUpdate (https://stateb.us/client6.js:3:23012)
  at t.performUpdateIfNecessary (https://stateb.us/client6.js:3:22627)
  at t.receiveComponent (https://stateb.us/client6.js:3:10486)
  at t.receiveComponent (https://stateb.us/client6.js:3:23235)
  at a._updateChildren (https://stateb.us/client6.js:5:12404)
  at a.updateChildren (https://stateb.us/client6.js:5:12119)
  at a._updateDOMChildren (https://stateb.us/client6.js:4:2795)
  at a.<anonymous> (https://stateb.us/client6.js:4:1396)
  at a.i [as updateComponent] (https://stateb.us/client6.js:5:15756)
  at a.performUpdateIfNecessary (https://stateb.us/client6.js:3:10727)
  at a.receiveComponent (https://stateb.us/client6.js:3:10486)
  at a.receiveComponent (https://stateb.us/client6.js:4:1191)
  at t.<anonymous> (https://stateb.us/client6.js:3:23471)
  at t.i [as updateComponent] (https://stateb.us/client6.js:5:15756)
  at t._performComponentUpdate (https://stateb.us/client6.js:3:23012)
  at t.performUpdateIfNecessary (https://stateb.us/client6.js:3:22627)
  at s (https://stateb.us/client6.js:5:26015)
  at r.perform (https://stateb.us/client6.js:6:11602)
  at o.perform (https://stateb.us/client6.js:6:11602)
  at o.perform (https://stateb.us/client6.js:5:27662)
  at Object.<anonymous> (https://stateb.us/client6.js:5:27878)
  at Object.i [as flushBatchedUpdates] (https://stateb.us/client6.js:5:15756)
  at r.closeAll (https://stateb.us/client6.js:6:12300)
  at r.perform (https://stateb.us/client6.js:6:11685)
  at Object.batchedUpdates (https://stateb.us/client6.js:4:15136)
  at Object.c [as enqueueUpdate] (https://stateb.us/client6.js:5:26670)
  at t.forceUpdate (https://stateb.us/client6.js:3:24093)
  at https://stateb.us/client6.js:13:2146

Here's the outcome when a new person joined (you can't see them) and then I opened a new window which made a new version of me (michael) who also didn't appear in the old window.

image

@tkriplean -- maybe you have an idea bout this bug.

karth295 commented 6 years ago

The exception is coming from here: https://github.com/invisible-college/tawk.space/blob/master/client/presence.coffee?utf8=%E2%9C%93#L169.

If I understand correctly, a /connection passed to this function has list of CSS selectors identifying everything your cursor is hovering over and an "index" specifying which one in case multiple match a selector. E.g. [("#foo", 0), (".bar", 1), ("video", 1)].

There's an obvious race condition where when the /connection was written, a selector existed at a particular index, but now no longer exists. Also, a selector could have existed for one client but not others. Note that presence.coffee gracefully deals with this by sending all things a client is hovering over, so you can still guess where their cursor is.

So the if statement that is supposed to guard against selectors not existing at an index is here: https://github.com/invisible-college/tawk.space/blob/master/client/presence.coffee?utf8=%E2%9C%93#L166.

Instead of:

if hover_els.length > 0 && hover_els[0]
      hovering_on = hover_els[(path.disambig_idx or 0)]

It should be

if hover_els[(path.disambig_idx or 0)]
      hovering_on = hover_els[(path.disambig_idx or 0)]
toomim commented 6 years ago

Wow! Karthik, you're my hero!

This makes total sense. I am happy to check this into tawk.space unless you want to.

@tkriplean -- do you know any other uses of presence.coffee that will want this fix?

It also sounds like it's about time for us to create a widget library where we can package these widgets and bugfix them for general use, so that bugfixes like this can be re-used by everyone. Does anyone want to tawk-meet sometime to design one? I'm happy to implement whatever design we come up with.

karth295 commented 6 years ago

You can go ahead and commit it.

I still vote we put all statebus apps in a single repo. If you change a widget then:

a) you have to run everybody's tests to make sure you didn't break things b) everybody gets the fix immediately

In addition, everybody will be more encouraged to write tests so they can ensure changes in dependencies don't break them.

I'm happy to tawk today or anytime this weekend before 5pm

tkriplean commented 6 years ago

Thanks Karthik! I updated my copy.

I'm also happy to tawk about widget library. I wouldn't mind doing what Karthik suggests and just have a statebus-widgets repo.

karth295 commented 6 years ago

I was thinking applications too, something like:

statebus/
  statebus.js
  server.js
  client.js
  # ...diffsync, etc

client/ # All libraries/widgets (.coffee)
  tawk/
    tawk.coffee
  presence/
    presence.coffee

server/  # idk if we have any shared server libraries yet
  # stuff

applications/ # Everything else
  tawk/
    index.js
    index.html
    favicon.ico
    # ... etc

That way applications are versioned with widgets -- I can make changes to tawk's index.html and tawk's tawk.coffee at the same time.

It would be nice to have statebus in this repo too. Right now, statebus is versioned separately from applications, meaning:

a) applications pin to a particular statebus version and don't get bugfixes without upgrading (and finding breaking changes when upgrading), or b) pin to head and potentially be broken by statebus changes immediately

If we're serious about sharing code and growing the ecosystem I think this will make life a lot easier. Idk how this plays into the marketplace concept though.

toomim commented 6 years ago

This is a good explanation, thank you! It's enlightening. I'm organizing my thoughts.