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(buckets): upgrade abstract interfaces, switch chat to use SharedBucket class #3285

Merged
merged 12 commits into from
Jun 9, 2022

Conversation

josephmcg
Copy link
Contributor

@josephmcg josephmcg commented May 24, 2022

What this PR does πŸ“–

  • improve RFM interface and create abstract Bucket class
    • PersonalBucket for personal, encrypted files
    • SharedBucket for unencrypted files such as profile picture, chat files
    • I considered combining them and adding various return types, but it seems sloppy. this has better type safety and will be easier to scale as the buckets diverge with more features
  • remove legacy BucketManager
  • add bucket path (id) to FileMessage payload. We were previously using file name for the path. However, if a new file with the same name is uploaded, it will overwrite the old file. using uuid for path will ensure this doesn't happen
  • switch chat to use streamsaver for downloads rather than blobs
  • move filesystem import into PersonalBucket init rather than account actions.ts

Which issue(s) this PR fixes πŸ”¨
AP-1607

Special notes for reviewers πŸ—’οΈ

  • There is currently no index to keep track of your shared uploads, that will be a future ticket
  • chat upload may behave strangely for large files still. that component needs additional updates, but I wanted to handle it separately. please ensure that small file uploads work in chat
  • message component still keeps track of image blobs, but doesn't use them for download. will be ripped out in future update
  • file save/download in chat will only work for newly uploaded files. previously uploaded files don't have id, so it won't be able to find the path

Additional comments 🎀

@josephmcg josephmcg added depending on other PR Blocked by other PR. Once the other PR has gone in, rebase this branch to dev and test. draft A developer wants eyes on this PR, but they don't think it's ready to merge. labels May 24, 2022
@netlify
Copy link

netlify bot commented May 24, 2022

βœ… Yeeeehaw, deploy preview is ready!

Name Link
πŸ”¨ Latest commit 8e47633
πŸ” Latest deploy log https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/62a13db01d626400099d9ea9
😎 Deploy Preview https://deploy-preview-3285--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.

@josephmcg josephmcg force-pushed the AP1607 branch 4 times, most recently from 4de5bbf to 1bbe512 Compare May 27, 2022 02:24
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label May 27, 2022
@josephmcg josephmcg removed missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa depending on other PR Blocked by other PR. Once the other PR has gone in, rebase this branch to dev and test. labels May 30, 2022
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label May 30, 2022
@github-actions github-actions bot removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label May 31, 2022
@josephmcg josephmcg 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 May 31, 2022
@josephmcg josephmcg marked this pull request as ready for review May 31, 2022 02:41
@stavares843 stavares843 force-pushed the AP1607 branch 3 times, most recently from d42f2a2 to 02d943d Compare June 3, 2022 16:16
@phillsatellite
Copy link
Contributor

Tested and found some issues that I haven't been able to replicate on dev. I got stopped short from Solana but I'll continue once its back up

When in a chat, each user will have the same color profile avatar

same color avatars

Other issue was I'm not seeing replies or reaction appear on other users side. I thought this might be textile but I wasn't able to replicate it on dev branch

missing reply

@phillsatellite phillsatellite added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Jun 3, 2022
@josephmcg
Copy link
Contributor Author

the avatar color was resolved in a separate PR, should be ok since Sara rebased this branch.
I'm not sure what's going on with the missing replies, I didn't alter anything that would affect those. I'll take a look once I'm able to test

@josephmcg josephmcg removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Jun 6, 2022
@molimauro molimauro added Ready for QA Ready for QA team to test, Devs approved. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Jun 8, 2022
@stavares843
Copy link
Member

the missing replies seems just the textile listener issue getting inactive and not related to this

@phillsatellite phillsatellite added QA tested One QA Person has tested - Needs QA Lead review still and removed Ready for QA Ready for QA team to test, Devs approved. labels Jun 8, 2022
@stavares843 stavares843 added temporary blocked checking something QA Lead is checking something. and removed QA tested One QA Person has tested - Needs QA Lead review still labels Jun 9, 2022
@stavares843 stavares843 merged commit defd500 into dev Jun 9, 2022
@stavares843 stavares843 deleted the AP1607 branch June 9, 2022 00:52
@github-actions github-actions bot removed the temporary blocked checking something QA Lead is checking something. label Jun 9, 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.

None yet

6 participants