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: auto-download blob namespace #722

Merged
merged 2 commits into from
Jul 24, 2024
Merged

Conversation

EvanHahn
Copy link
Contributor

Media isn't syncing. This seems to be because we hard-coded the blob namespace not to auto-download. This removes that and adds an automated test to check it.

I don't know why we did this, but it seems to have originated in a 2023 commit (later updated in another commit).

Should address comapeo-mobile#514.

Media isn't syncing. This seems to be because we hard-coded the `blob`
namespace not to auto-download. This removes that and adds an automated
test to fix it.

I don't know why we did this, but it seems to have originated in [a 2023
commit][0] (later updated in [another commit][1]).

Should address [comapeo-mobile#514].

[0]: 5a47b56
[1]: ea442a3
[comapeo-mobile#514]: digidem/comapeo-mobile#514
@EvanHahn EvanHahn requested a review from achou11 July 24, 2024 01:01
@EvanHahn
Copy link
Contributor Author

@achou11 Would you mind testing this in CoMapeo when you get a chance? I want to make sure this actually solves the real problem.

@achou11
Copy link
Member

achou11 commented Jul 24, 2024

Yeah seems to fix the specific issue of syncing blobs! Was able to get blobs synced under the following conditions:

  1. Device A creates project and adds Device B to project.
  2. Device A creates observation (with photo attachment)
  3. Device B creates observation (with or without photo).
  4. Both devices go to sync screen
  5. Both devices detect syncable changes, both enable sync

There's a separate sync issue that I've discovered but I think it's unrelated to this change, so I would lean towards saying that it does fix the issue in question.

@EvanHahn EvanHahn marked this pull request as ready for review July 24, 2024 18:37
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

infuriatingly small fix

@achou11 achou11 merged commit f5591d5 into main Jul 24, 2024
7 checks passed
@achou11 achou11 deleted the auto-download-blob-namespace branch July 24, 2024 18:53
@gmaclennan
Copy link
Member

I don't know why we did this, but it seems to have originated in a 2023 commit (later updated in another commit).

@EvanHahn @achou11 this was done this way before we decided to drop selective sync of blobs from MVP. The intention was to control blob download via blobStore.download(). However, since we are downloading all blobs for MVP, calling core.download({ start: 0, end: -1 }) does the same thing as blobStore.download() without a filter, e.g. this is an suitable fix for now. When we do add the media management feature to download only previews, then we can revert this change and add functionality to turn on blob downloading according to the user setting.

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