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

feat(indexedDB): clean up searchIndex, indexeddb #2419

Merged
merged 1 commit into from
Mar 23, 2022
Merged

feat(indexedDB): clean up searchIndex, indexeddb #2419

merged 1 commit into from
Mar 23, 2022

Conversation

aewing
Copy link
Contributor

@aewing aewing commented Mar 18, 2022

What this PR does 📖

  • Introduces the libraries/SatelliteDB library in place of the thirdparty dexie plugin (will revisit plugins as time allows)
  • Refactors the SearchIndex class a bit, leveraging minisearch in place of the previous dexie
  • Syncs new conversations, messages, and friends to indexeddb and search indexes

Which issue(s) this PR fixes 🔨
AP-979

Special notes for reviewers 🗒️

Additional comments 🎤
Feel free to reach out directly with any questions to try to keep the issue clean for other reviewers.

@aewing aewing marked this pull request as draft March 18, 2022 21:31
@netlify
Copy link

netlify bot commented Mar 18, 2022

✅ Yeeeehaw, deploy preview is ready!

🔨 Explore the source changes: c350e49

🔍 Inspect the deploy log: https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/6234f9007322140008b80cda

😎 Browse the preview: https://deploy-preview-2419--adoring-edison-dbcef8.netlify.app

@netlify
Copy link

netlify bot commented Mar 18, 2022

Yeeeehaw, deploy preview is ready!

Name Link
🔨 Latest commit c4d6682
🔍 Latest deploy log https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/623a228d226c6b0008a01ea9
😎 Deploy Preview https://deploy-preview-2419--adoring-edison-dbcef8.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@stavares843 stavares843 added the draft A developer wants eyes on this PR, but they don't think it's ready to merge. label Mar 19, 2022
@aewing aewing marked this pull request as ready for review March 19, 2022 01:03
@aewing aewing added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed draft A developer wants eyes on this PR, but they don't think it's ready to merge. labels Mar 19, 2022
@stavares843
Copy link
Member

/rebase

@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 21, 2022
@stavares843 stavares843 removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 21, 2022
Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

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

Captura de ecrã 2022-03-21, às 19 04 42

can you clarify exactly what is this PR adding related to the search?

I send messages saying ''a'', searched ''a'', no results found

is the search expected to be working on this PR?

@stavares843 stavares843 added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Mar 21, 2022
Copy link
Contributor

@josephmcg josephmcg left a comment

Choose a reason for hiding this comment

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

I'll let Drew confirm, but I would guess it only searches after a certain character threshold. Searching for a single letter like 'a' wouldn't be very helpful since it would return any word with an 'a'. This seems like an under-the-hood change, UI updates coming in a later PR

@stavares843
Copy link
Member

well, last time this was tested the search was far from perfect

also, that info is nowhere to be found on the PR, if it has or not a threshold which doesn't even make sense having that because if a user wants to search only by a number or an emoji, he couldn't due to the threshold

@stavares843
Copy link
Member

Captura de ecrã 2022-03-22, às 01 27 48

tested again and ''a'' worked

but then tested ''test'' and crashed

Captura de ecrã 2022-03-22, às 01 28 05

then sent ''SATELLITE'' and searched again by ''SATELLITE'' and crashed

so this crash's everything you send something > search for that > crash

reload, crash is gone, send other keyword > search for that one > crash's again

@stavares843
Copy link
Member

also, both locally and CI the imported chats gets broken

Captura de ecrã 2022-03-22, às 04 08 11

is this something expected within this PR?

doesn't seem to be any useful error on the log

@stavares843
Copy link
Member

stavares843 commented Mar 22, 2022

i also have that same issue with dexie when importing specific accounts, more details here

@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 22, 2022
@aewing
Copy link
Contributor Author

aewing commented Mar 22, 2022

also, both locally and CI the imported chats gets broken

Captura de ecrã 2022-03-22, às 04 08 11

is this something expected within this PR?

doesn't seem to be any useful error on the log

I'm not seeing this issue when importing my account. When I attempt to import the seed phrase provided, I see a series of "Bad API key" messages locally, and it works in the dev environment.

@stavares843
Copy link
Member

very strange

thanks for checking 🔨

@aewing
Copy link
Contributor Author

aewing commented Mar 22, 2022

Captura de ecrã 2022-03-22, às 01 27 48

tested again and ''a'' worked

but then tested ''test'' and crashed

Captura de ecrã 2022-03-22, às 01 28 05

then sent ''SATELLITE'' and searched again by ''SATELLITE'' and crashed

so this crash's everything you send something > search for that > crash

reload, crash is gone, send other keyword > search for that one > crash's again

Thanks for finding this. I was able to reproduce this issue and have added a fix (appended the conversation address to new messages before adding them to the index, so the user can be attached in the UI)

@github-actions github-actions bot removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 22, 2022
@WanderingHogan
Copy link
Contributor

@stavares843 dont worry about anything in search, we have an upcoming task for Joe to refactor/work that

@WanderingHogan WanderingHogan added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Mar 22, 2022
@stavares843 stavares843 added the temporary blocked checking something QA Lead is checking something. label Mar 22, 2022
@stavares843
Copy link
Member

dev

Gravacao.do.ecra.2022-03-22.as.22.50.33.mov

this branch

test0000001.mov

on dev, you send a glyph - that appears on the front of you
on this branch you send a glyph - that doesn't appear in front of you

@stavares843 stavares843 added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. temporary blocked checking something QA Lead is checking something. labels Mar 22, 2022
@WanderingHogan
Copy link
Contributor

i can't for the life of me replicate the problem you are having

@stavares843
Copy link
Member

so shady

@stavares843
Copy link
Member

stavares843 commented Mar 22, 2022

Gravacao.do.ecra.2022-03-22.as.23.45.27.mov

flow

  • go to dev
  • send something
  • appears in front of you (A)
  • scroll up
  • send something - will not appear in front of you, we don't have that type of scroll added yet (B)
  • scroll down
  • send something
  • appears in front of you (because you were all scrolled down like the A above)

then

  • go to this branch
  • send something
  • appears in front of you (A)
  • scroll up
  • send something - will not appear in front of you, we don't have that type of scroll added yet (B)
  • scroll down
  • send something
  • It DOESN'T appears in front of you (and you were all scrolled down like the A above so should be in front of you like dev)

@aewing
Copy link
Contributor Author

aewing commented Mar 23, 2022

Gravacao.do.ecra.2022-03-22.as.23.45.27.mov

flow

* go to dev

* send something

* appears in front of you (A)

* scroll up

* send something - will not appear in front of you, we don't have that type of scroll added yet (B)

* scroll down

* send something

* appears in front of you (because you were all scrolled down like the A above)

then

* go to this branch

* send something

* appears in front of you (A)

* scroll up

* send something - will not appear in front of you, we don't have that type of scroll added yet (B)

* scroll down

* send something

* It DOESN'T appears in front of you (and you were all scrolled down like the A above so should be in front of you like dev)

just tested this on #2407 and a few other branches and the issue is consistent across the open PRs right now.

@stavares843
Copy link
Member

on dev, is always good for me

Phil is checking as well

@aewing
Copy link
Contributor Author

aewing commented Mar 23, 2022

Satellite-Absolute.Mozilla.Firefox.2022-03-22.18-26-59.mp4

on dev.satellite.one

@WanderingHogan WanderingHogan added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 23, 2022
@stavares843 stavares843 merged commit d78744d into dev Mar 23, 2022
@stavares843 stavares843 deleted the AP-979 branch March 23, 2022 01:53
@github-actions github-actions bot removed the Ready for QA Ready for QA team to test, Devs approved. label Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants