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

Mobile: Fixes #11134: Fix automatic resource download mode #11144

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Sep 27, 2024

Summary

Previously, automatic resource download was handled on component mount by checking the body of this.state.note. However at this point, this.state.note is not set (setState has been called, but this doesn't seem to set this.state.note immediately).

This pull request moves the automatic resource download check to componentDidUpdate, when state.isLoading goes from false to true. At present, isLoading is set to true initially, then set to false after loading a note.

This pull request should fix #11134.

Important

Changes from #11145 is included in this pull request. This is to prevent CI failures related to new tests in this pull request. As such, most changes in this pull request are test-related.

Consider reviewing #11145 before this pull request.

If desired, I can create a separate pull request with just the change to Note.tsx that fixes the bug, without unit tests.

Testing plan

This pull request adds an automated test.

Additionally, it has been tested manually on web (Chromium) by:

  1. Attaching a new resource to a note on the desktop app.
  2. Syncing the desktop app.
  3. Switching to the mobile app (web, attachment download behavior set to "auto").
  4. Syncing the mobile app.
  5. Opening the modified note.
  6. Verifying that the resource icon is shown as an hourglass.
  7. Verifying that the resource hourglass icon is replaced with the resource.

@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Sep 28, 2024

The related CI failures may be fixed by #11145. Edit: I've merged #11145 into this branch to allow the CI to pass.

@personalizedrefrigerator personalizedrefrigerator added the mobile All mobile platforms label Sep 28, 2024
@laurent22 laurent22 merged commit b614670 into laurent22:dev Oct 11, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile All mobile platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile: Automatic resource download not working
2 participants