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 (remote assets): handle asset pack containing asset overrides #2408

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

jfmcquade
Copy link
Collaborator

@jfmcquade jfmcquade commented Sep 11, 2024

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

Remote assets: Adds functionality to handle case of an asset pack containing asset overrides (previously only top-level/"default" assets were supported).

See below for demo.

TODO/Next steps

  • Generating asset packs and manifest
    • I have been doing this manually, we'll want a workflow(?) to handle bundling targeted assets into an asset pack, generating a manifest in FlowTypes.AssetPack format, and uploading everything to supabase
  • Debug iOS
    • There are some issues with retrieving files from downloaded asset packs from local storage on iOS. I think a separate follow-up PR makes sense for this issue.
  • Further details are included in the RFC, which should be updated after merge

Git Issues

Closes #

Screenshots/Videos

The template debug_remote_assets captures various use cases. Essentially, there are the following asset files included in either the "core" assets (standard assets included in the app bundle) or the "debug_asset_pack_1" asset pack (uploaded to supabase):

Included in core assets Included in debug_asset_pack_1
Image 1 image_1 image_1
Image 2 No file image_2
Image 3 (global variant) image_3 No file
Image 3 (es_sp variant) No file image_3
Image 4 (global variant) image_4 image_4
Image 4 (es_sp variant) image_4 image_4

Google Drive folder structure:
Screenshot 2024-09-25 at 13 28 14

Supabase storage folder structure:
Screenshot 2024-09-25 at 13 27 12

debug_remote_assets:

Screenshot 2024-09-25 at 13 29 52

Web demo:

web.mov

Android demo:

Android.mov

@@ -133,6 +133,14 @@ export interface IDeploymentConfig {
_parent_config?: Partial<IDeploymentConfig & { _workspace_path: string }>;
}

/** Duplicate type defintion from data-models (TODO - find better way to share) */
interface IFlowTypeBase {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved from lower down to group with other relevant definitions

@@ -18,7 +18,7 @@
.popup-container {
height: var(--safe-area-height);
}
.close-button {
.close-button {
Copy link
Collaborator Author

@jfmcquade jfmcquade Sep 23, 2024

Choose a reason for hiding this comment

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

Not sure why this is cropping up now, but reverting it and committing isn't possible as the linter removes the whitespace again when I attempt a commit (so the would-be commit is empty)

@jfmcquade jfmcquade changed the title Feat: remote assets handle asset overrides Feat (remote assets): handle asset pack containing asset overrides Sep 25, 2024
@esmeetewinkel
Copy link
Collaborator

@jfmcquade It looks to me like the debug storage bucket on Supabase sits within a project named plh. Should we rename that open-app-builder?

image

@jfmcquade
Copy link
Collaborator Author

I added the test - preview label to the PR to test on web, but got an error saying the debug deployment config will need updating first. @jfmcquade Could you update the debug config as an example? I guess I could then test by adding the test - preview label to that content repo PR (ensuring that the debug repo targets this code PR branch).

That's strange, the debug config on the main branch should already be configured correctly. I get the same error on the live web preview, but not locally. I suspect this is something to do with the way that the encrypted supabase config is supposed to be decrypted and incorporated into the deployment config. @chrismclarke can you think of why this might not be working when the deployment is set as part of the github action? Looking at the logs for the deployment preview action triggered by this PR (see "Set deployment" section of "build"), I can see that initially there are logs complaining that the encrypted configs do not exist, but this is before the deployment has been set. Then when the deployment is set, the encrypted configs do appear to be decrypted successfully, so I may be misdiagnosing the issue.

@jfmcquade
Copy link
Collaborator Author

@jfmcquade It looks to me like the debug storage bucket on Supabase sits within a project named plh. Should we rename that open-app-builder?

I think we probably should. We can specify a target supabase project via the deployment config, so in theory it would be a good idea to keep separate projects for different deployments, or at least groups of deployments like PLH. However, as supabase only allows an organisation to have two free projects at once, it may well make sense to, at least initially, start with a single "open app builder" project that we can use to store everything.

Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

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

Thanks for adding this. I really like the debug template you've setup, does a very good job of illustrating core functionality, although I appreciate it's a bit tricky to manage differences between web and native.

I've added one minor tidying PR, just to move the asset type definitions to their own file
5fdf023, and have confirmed that the tests and debug sheet are working locally.

I'm not sure quite what's next for this feature, although I would recommend when next working on the code that it might be worth trying to split out the remote-asset.service code a bit as the file is getting quite long and hard to keep track of what is and isn't being tested. It might be worth separating the actions to a remote-asset.actions.ts with corresponding remote-asset.actions.spec.ts and possibly there might be some use having a remote-asset.utils.ts to handle methods that are purely data manipulation (no outside service interaction).

I'd also recommend splitting out the code that initialises supabase into a supabse.service.ts

Somewhat unrelated, I can see the RemoteAssetService is initialised in the main app as a blocking service. Given that most of the interaction will be from user-generated actions I'd say likely this can be deferred to after main init.

Oh, and I noticed the inline comment about queueing parallel downloads - agreed this would be nice to have although before getting to that point it might be worth considering some sort of UI/popup to display that shows the overall progress of the remote asset download, as I can't really think of a tidy way to include as a variable within templates at this point in time (could be configured later)

@chrismclarke
Copy link
Member

chrismclarke commented Oct 9, 2024

That's strange, the debug config on the main branch should already be configured correctly. I get the same error on the live web preview, but not locally. I suspect this is something to do with the way that the encrypted supabase config is supposed to be decrypted and incorporated into the deployment config. @chrismclarke can you think of why this might not be working when the deployment is set as part of the github action? Looking at the logs for the deployment preview action triggered by this PR (see "Set deployment" section of "build"), I can see that initially there are logs complaining that the encrypted configs do not exist, but this is before the deployment has been set. Then when the deployment is set, the encrypted configs do appear to be decrypted successfully, so I may be misdiagnosing the issue.

Ah, yeah I see the issue. I get the same problem locally when importing the debug repo for the first time. In order to decrypt the repo the repo first needs to be set (hence on first load it says encrypted not found), but then after being set and decrypted there's no update to the config.ts file so it uses the cached config.json

Hopefully fixed with #2462

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scripts Work on backend scripts and devops test - preview Create a preview deployment of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants