feat(partial-update): add support for partial update to Capgo #353
Conversation
✅ Deploy Preview for webcapgo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "prod": "https://xvwzpoazmxkqosrdewyv.supabase.co", | ||
| "development": "https://aucsybvnhavogdmzwtcw.supabase.co", | ||
| "prod": "http://localhost:54321", | ||
| "development": "http://localhost:54321", | ||
| "local": "http://localhost:54321" | ||
| } | ||
| } |
There was a problem hiding this comment.
🤖 ChatGPT: Bug risk: There doesn't seem to be any bug introduced by this patch.
Improvement suggestion: Consider using environment variables or configuration files to store URLs instead of hardcoding them directly in the code. This would make it easier to change the URLs without modifying the code itself.
LGTM! The patch is acceptable as it updates the URLs for prod, development, and local environments.
There was a problem hiding this comment.
This probably should not have been committed
There was a problem hiding this comment.
IMHO, it's totally harmless especially since it's how everyone deploys the app locally anyway.
There was a problem hiding this comment.
But can you guarantee this will not crash production?
There was a problem hiding this comment.
@ayewo this is used in prod, can you update it ?
| epv1zj0xR7z2PdimYr3Kon9cHecLgvJ6p4Bi8io/wJskBOZOuFUIcHTnW5j1ciun | ||
| iMkyEfT132c0Tx/sjVfLM5j0db3h7F4dD0GynvRB0IIl0/EH42fEGqD+mRXKKRv8 | ||
| X/ijXs/JpSpSyRdPBpUUnUE= | ||
| -----END PRIVATE KEY----- |
There was a problem hiding this comment.
🤖 ChatGPT: Code Review:
The patch adds a private key file named localhost+2-key.pem with its content.
Bug Risk:
There doesn't seem to be any specific bug risk associated with this patch. However, it's important to ensure that the private key is used securely and protected from unauthorized access.
Improvement Suggestion:
Consider storing sensitive information like private keys in a secure location outside of the source code repository, such as a secrets management system or environment variables, to enhance security.
OK
| 2jIq71HdCxf0NDWuPCUil983Nb75j+P/tyTQPEHuCowZ6IMZqkDr/RIbnNbhLTWM | ||
| 7vaS2VXpwQfBJCz3jzBzEBl7iFBxoXkV+dYbSEZE1MwBI8fZGbfFUwxczV2XHqEM | ||
| usnYGZXKfHHOCk9oAT2FEFzv/01emAfOiLJLGMWPAfHQ/j5NO3jDOZWSqIT4 | ||
| -----END CERTIFICATE----- |
There was a problem hiding this comment.
🤖 ChatGPT: This patch adds a new certificate file, localhost+2.pem, with its content provided as a diff snippet. Without the context of the codebase and its usage, it is difficult to assess any bug risks or improvement suggestions related to this specific certificate file. If you require a more detailed review, please provide additional information about its purpose and how it is used in the codebase.
| @@ -7,6 +7,9 @@ | |||
|
|
|||
| [dev] | |||
| framework = "vite" | |||
| [dev.https] | |||
| certFile = "localhost+2.pem" | |||
| keyFile = "localhost+2-key.pem" | |||
|
|
|||
| [[redirects]] | |||
| from = '/.well-known/*' | |||
There was a problem hiding this comment.
🤖 ChatGPT: Bug risk: It seems like this patch adds HTTPS configuration for the development environment using self-signed certificates. While this may be necessary for local testing, it could introduce security risks if used in a production environment without proper certificate management.
Improvement suggestion: Consider adding a comment explaining the purpose of the HTTPS configuration and mentioning that it should only be used for development/testing purposes.
There was a problem hiding this comment.
What is the point of this? I know that HTTP does now work with android and there is a check for that. How exactly have you tested your code? Why was HTTPS required?
There was a problem hiding this comment.
For iOS.
ATS (App Transport Security) blocks HTTP by default.
The workaround to enable HTTP is messy so I used mkcert to create a self-signed SSL cert.
There was a problem hiding this comment.
Can you disable ATS locally? Without this change
There was a problem hiding this comment.
You can, you also can disable HTTPS only in android so this change is not required
| console.log(`Cannot verify user ${JSON.stringify(userIdError)}`); | ||
| } | ||
| return userId; | ||
| } |
There was a problem hiding this comment.
🤖 ChatGPT: Bug risk: None apparent.
Improvement suggestion: Consider using more descriptive variable names for better code readability.
| @@ -1,5 +1,5 @@ | |||
| import { serve } from 'https://deno.land/std@0.200.0/http/server.ts' | |||
| import { isAdmin, supabaseAdmin } from '../_utils/supabase.ts' | |||
| import { isAdmin, supabaseAdmin, verifyUser } from '../_utils/supabase.ts' | |||
| import { sendOptionsRes, sendRes } from '../_utils/utils.ts' | |||
| import { getBundleUrl } from '../_utils/downloadUrl.ts' | |||
|
|
|||
There was a problem hiding this comment.
🤖 ChatGPT: Code Review: supabase/functions/download_link/index.ts
-
Bug Risk:
- The condition check for
authorizationis improved by checking if the token exists usingauthorization?.split('Bearer ')[1]. However, it would be better to validate the token's format and handle any potential errors more explicitly. - There is a missing error handling for the case when
supabaseAdmin().auth.getUser()encounters an error. It should be handled appropriately.
- The condition check for
-
Improvement Suggestions:
- The comment
// TODO: fix for adminshould be addressed and elaborated upon to provide further context on what needs fixing and how it will be resolved.
- The comment
OK
| console.log('url', filePath, url) | ||
| return sendRes({ url }) | ||
| } | ||
| catch (e) { | ||
| return sendRes({ | ||
| status: 'Error unknow', | ||
| status: 'Error unknown', | ||
| error: JSON.stringify(e), | ||
| }, 500) | ||
| } |
There was a problem hiding this comment.
🤖 ChatGPT: Bug risk: The error messages in the returns statements are inconsistent. In one case, it says "Error already exist" and in another case, it says "Error unknow." This inconsistency could lead to confusion and make troubleshooting difficult.
Improvement suggestion: It would be better to provide more specific error messages that clearly indicate the nature of the error. For example, instead of "Error unknow," you could provide a descriptive message like "Error: Unknown error occurred." Similarly, instead of "Error already exist," you could mention the specific error, such as "Error: File already exists."
OK
|
🤖 ChatGPT score: Based on the provided diff, I am unable to determine a score for the pull request. The diff includes changes related to URLs and certificates but does not provide sufficient context to evaluate the overall impact or correctness of the changes. |
| if (cliUserId) { | ||
| console.assert(body.user_id == cliUserId) | ||
| } else { | ||
| return sendRes({ status: 'Not authorized. The API key is invalid' }, 401) |
There was a problem hiding this comment.
This will never fail, you allays return from verifyUser
There was a problem hiding this comment.
Right, but if the userId is empty, then this block can fail.
| if (!authorization) | ||
| return sendRes({ status: 'Cannot find authorization' }, 400) | ||
| if (!authorization) { | ||
| return sendRes({ status: 'Cannot find authorization token in header' }, 400) |
There was a problem hiding this comment.
please lint the code with pnpm run lint
There was a problem hiding this comment.
I ran pnpm run lint and no changes were made for me to commit.
When you run it, what would it have fixed?
There was a problem hiding this comment.
Okay, turns out it is a problem that is unrelated to your PR. You do not have automatic linting enabled in your editor. For now do not worry about this. I will create an issue for linting as linting does not trigger for the supabase folder and I have automatic linting enabled so I just cough that this is not linted properly
There was a problem hiding this comment.
This is exactly how I saw it in my head and how my automatic linting (configured based on capgo settings) linted it
| "prod": "https://xvwzpoazmxkqosrdewyv.supabase.co", | ||
| "development": "https://aucsybvnhavogdmzwtcw.supabase.co", | ||
| "prod": "http://localhost:54321", | ||
| "development": "http://localhost:54321", | ||
| "local": "http://localhost:54321" | ||
| } | ||
| } |
There was a problem hiding this comment.
This probably should not have been committed
| @@ -7,6 +7,9 @@ | |||
|
|
|||
| [dev] | |||
| framework = "vite" | |||
| [dev.https] | |||
| certFile = "localhost+2.pem" | |||
| keyFile = "localhost+2-key.pem" | |||
|
|
|||
| [[redirects]] | |||
| from = '/.well-known/*' | |||
There was a problem hiding this comment.
What is the point of this? I know that HTTP does now work with android and there is a check for that. How exactly have you tested your code? Why was HTTPS required?
| const userId = (dataUser || '').toString(); | ||
|
|
||
| if (!userId || userIdError) { | ||
| console.log(`Cannot verify user ${JSON.stringify(userIdError)}`); |
There was a problem hiding this comment.
There should be a return here right? Or if not then lint this
There was a problem hiding this comment.
Yea, it's on line 612.
There was a problem hiding this comment.
an empty string is not a valid user id, why not just return undefined on error?
| @@ -20,6 +20,7 @@ export interface AppInfos { | |||
| platform: string | |||
| app_id: string | |||
| device_id: string | |||
| partial_update?: boolean | |||
| } | |||
| export interface AppStats extends AppInfos { | |||
| action: string | |||
There was a problem hiding this comment.
🤖 ChatGPT: Bug risk: None.
Improvement suggestion: The addition of the partial_update property in the AppInfos interface looks fine. However, it would be helpful to provide more context about its purpose, usage, and any constraints in the code comments or documentation.
LGTM! The addition of the partial_update property in the AppInfos interface seems appropriate for supporting partial updates.
| @@ -20,6 +20,7 @@ export interface AppInfos { | |||
| platform: string | |||
| app_id: string | |||
| device_id: string | |||
| partial_update?: boolean | |||
| } | |||
| export interface AppStats extends AppInfos { | |||
| action: string | |||
There was a problem hiding this comment.
🤖 ChatGPT: Code Review:
Bug Risk: No apparent bug risks in this patch.
Improvement Suggestion: None.
LGTM! The addition of the partial_update field to the AppInfos interface and the Manifest interface looks good.
| import type { Database } from '../_utils/supabase.types.ts' | ||
| import { sendNotif } from '../_utils/notifications.ts' | ||
| import { getBundleUrl } from '../_utils/downloadUrl.ts' | ||
| import { logsnag } from '../_utils/logsnag.ts' | ||
| import { appIdToUrl } from './../_utils/conversion.ts' | ||
|
|
||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row']) { | ||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row'], manifest?: Manifest) { | ||
| const res: any = { | ||
| version: version.name, | ||
| url: signedURL, |
There was a problem hiding this comment.
🤖 ChatGPT: Bug risk: The code change introduces a new parameter manifest to the function resToVersion(). Since this is a function that is called elsewhere in the codebase, there is a risk that the introduction of the new parameter may cause issues if the function is not properly updated where it is used.
Improvement suggestion: It would be beneficial to ensure that all usages of the resToVersion() function are also updated to pass the manifest parameter when necessary. This will help maintain consistency and prevent potential errors.
| import type { Database } from '../_utils/supabase.types.ts' | ||
| import { sendNotif } from '../_utils/notifications.ts' | ||
| import { getBundleUrl } from '../_utils/downloadUrl.ts' | ||
| import { logsnag } from '../_utils/logsnag.ts' | ||
| import { appIdToUrl } from './../_utils/conversion.ts' | ||
|
|
||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row']) { | ||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row'], manifest?: Manifest) { | ||
| const res: any = { | ||
| version: version.name, | ||
| url: signedURL, |
There was a problem hiding this comment.
🤖 ChatGPT: Bug risk: No apparent bug risks in this patch.
Improvement suggestion:
Consider adding type annotations for the res object in the resToVersion function to improve code clarity and maintainability.
| import type { Database } from '../_utils/supabase.types.ts' | ||
| import { sendNotif } from '../_utils/notifications.ts' | ||
| import { getBundleUrl } from '../_utils/downloadUrl.ts' | ||
| import { logsnag } from '../_utils/logsnag.ts' | ||
| import { appIdToUrl } from './../_utils/conversion.ts' | ||
|
|
||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row']) { | ||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row'], manifest?: Manifest) { | ||
| const res: any = { | ||
| version: version.name, | ||
| url: signedURL, |
There was a problem hiding this comment.
🤖 ChatGPT: Bug Risk:
- The code appears to have a bug risk. In the
resToVersionfunction, themanifestparameter is optional (manifest?: Manifest), but it's used without checking if it exists (if (semver.gte(plugin_version, '5.3.0') && manifest)). This could lead to an error if themanifestis not provided when it's expected.
Improvement Suggestions:
- Consider adding more descriptive variable names for parameters in the
updatefunction, such ascustomIdinstead ofcustom_idandisEmulatorinstead ofis_emulator, for better readability. - It would be helpful to include comments or documentation explaining the purpose and usage of the
partial_updateparameter in theupdatefunction.
Please note that these suggestions are based solely on the provided code snippet and may not take into account the context or requirements of the larger codebase.
|
Kudos, SonarCloud Quality Gate passed!
|
| import type { Database } from '../_utils/supabase.types.ts' | ||
| import { sendNotif } from '../_utils/notifications.ts' | ||
| import { getBundleUrl } from '../_utils/downloadUrl.ts' | ||
| import { logsnag } from '../_utils/logsnag.ts' | ||
| import { appIdToUrl } from './../_utils/conversion.ts' | ||
|
|
||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row']) { | ||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row'], manifest?: Manifest) { | ||
| const res: any = { | ||
| version: version.name, | ||
| url: signedURL, |
There was a problem hiding this comment.
🤖 ChatGPT: Bug Risk:
- The
getSignedURLfunction is not imported, so there might be an error when calling it later in the code.
Improvement Suggestion:
- It would be helpful to include a comment or documentation explaining the purpose and functionality of the
partial_updatevariable. - Consider using more descriptive variable names to improve code readability.
OK
| import type { Database } from '../_utils/supabase.types.ts' | ||
| import { sendNotif } from '../_utils/notifications.ts' | ||
| import { getBundleUrl } from '../_utils/downloadUrl.ts' | ||
| import { logsnag } from '../_utils/logsnag.ts' | ||
| import { appIdToUrl } from './../_utils/conversion.ts' | ||
|
|
||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row']) { | ||
| function resToVersion(plugin_version: string, signedURL: string, version: Database['public']['Tables']['app_versions']['Row'], manifest?: Manifest) { | ||
| const res: any = { | ||
| version: version.name, | ||
| url: signedURL, |
There was a problem hiding this comment.
🤖 ChatGPT: Bug risk: The getPartialUpdate function is missing the condition to filter by app_id, which means it will fetch partial updates for all applications instead of just the specific one. This can lead to incorrect results and potential security issues if unauthorized access is allowed.
Improvement suggestion: Add a condition in the getPartialUpdate function to filter by the app_id parameter to ensure only the relevant partial updates are fetched.
LGTM! Consider adding the mentioned improvement to the getPartialUpdate function to filter by app_id for better security and accuracy.
|
Hey @ayewo thanks for the huge work, can you merge main in your branch @WcaleNieWolny made the lint step for backend: |
|
A demo video and some explanation on how the logic work could be nice as well |
Part 3 of 3 PRs to that adds support for partial updating to Cap-go/capacitor-updater.