-
Notifications
You must be signed in to change notification settings - Fork 6
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
sagar / Refactor android config #78
Conversation
refactor: update host to `copy-trading.binary.sx`
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request refactors the Android project configuration to update the application ID, host name, launch URL, version code, version name, web manifest URL, and full scope URL. It also removes a redundant path prefix from the Android manifest. Flow diagram of TWA URL handling changesgraph TD
A[Android TWA App] -->|Opens URL| B{URL Handler}
B -->|Old Path| C[deriv-com.github.io/copy-trading/]
B -->|New Path| D[copy-trading.binary.sx/]
D -->|Simplified Path| E[Remove /copy-trading/ prefix]
C -->|Legacy| F[Web Manifest at /copy-trading/manifest.json]
D -->|Updated| G[Web Manifest at /manifest.json]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI Code Review
🔴 BUGS & LOGICAL ISSUES:
-
Missing path handling in Intent Filter
------------------------------------------------------------------
• Issue description: The for https traffic previously included android:pathPrefix="/copy-trading/", restricting the TWA to handle only the “/copy-trading/” path. This has now been removed, meaning all paths on copy-trading.binary.sx may be intercepted by the TWA. If your site’s routing logic or deep-link handling depends on the /copy-trading/ prefix, removing the path prefix could cause unexpected behaviors or collisions with other paths.
• Potential impacts: Unintended deep links might open in the TWA; 404s or incomplete redirect handling if a user attempts to visit a path that is implicitly assumed to still start at /copy-trading/.
• Reproduction scenarios:
– Attempt to open copy-trading.binary.sx/some-other-path which was never intended to open in TWA.
– Apps or shared intents that rely on /copy-trading/ prefix for routing may be broken.
• Fix implementation with code example:
If your app truly needs to handle all top-level paths, then this is correct. Otherwise, restore the path prefix: -
Possible HTTP-to-HTTPS mismatch
• Issue description: Some references were changed from http://… to https://…, but ensure that the new domain actually supports secure HTTPS connections for all relevant resources (icons, manifest JSON).
• Potential impacts: If the new domain or some asset requires HTTP or is not served properly via HTTPS, the TWA may fail to fetch the resources, causing broken icons or no manifest load.
• Reproduction scenarios:
– TWA freeze or default fallback if the icon URL at https://copy-trading.binary.sx/pwa-512x512.png is unreachable or incorrectly redirects.
– Mixed content warnings if something in the chain tries to use HTTP instead of HTTPS.
• Fix implementation with code example:
Before merging, verify the SSL coverage on copy-trading.binary.sx. In your build.gradle or twa-manifest.json, confirm all icons/manifest references share the same secure scheme. Example snippet:"iconUrl": "https://copy-trading.binary.sx/pwa-512x512.png",
"maskableIconUrl": "https://copy-trading.binary.sx/pwa-512x512.png",
"webManifestUrl": "https://copy-trading.binary.sx/manifest.json"Make sure these URLs are valid and served over HTTPS.
-
Potential versioning gap
• Issue description: The versionCode and versionName jumped from 5 to 9. While not a functional bug, large version jumps may cause confusion in your release history.
• Potential impacts: Automatic updates in the Play Store will proceed normally, but your release notes or build pipelines might see anomalies.
• Reproduction scenarios:
– Developers or QA expecting sequential version increments might be confused about missing 6–8 releases.
• Fix implementation with code example:
Ensure your release process is documented clearly, e.g.:defaultConfig {
versionCode 6 // or 9, with an explanation
versionName "6"
// ...
}
🟡 RELIABILITY CONCERNS:
-
Potential for TWA fallback scenario
• Edge cases identified: If the site is offline or the SSL certificate is invalid, the TWA might fail at launch.
• Potential failure scenarios: Users see a white screen or indefinite spinner rather than a graceful fallback or error message.
• Mitigation steps with code examples:
– Implement a fallback webview or offline page in your TWA settings. In your twa-manifest.json, set fallbackType to "customtabs" or "webview" and add any fallback logic:
------------------------------------------------------------------
"fallbackType": "webview",
// ...
"features": {
"preferCustomTab": true
},
------------------------------------------------------------------
– Provide a user-friendly offline message or fallback URL if the network fails. -
Cache invalidation and deployment synchronization
• Edge cases identified: The newly updated TWA and changed domain references might cause older device caches to point to /copy-trading/ or older manifest components.
• Potential failure scenarios: If the user’s device has an older cached service worker or manifest referencing /copy-trading/, they might fail to load the new route at “/.”
• Mitigation steps with code examples:
– In your PWA backend code, ensure the service worker handles new route requests or forcibly invalidates older caches:
------------------------------------------------------------------
// service-worker.js
self.addEventListener('activate', (event) => {
const cacheWhitelist = ['my-new-cache-name'];
event.waitUntil(
caches.keys().then(cacheNames => {
return Promise.all(
cacheNames.map(cacheName => {
if (!cacheWhitelist.includes(cacheName)) {
return caches.delete(cacheName);
}
})
);
})
);
});
------------------------------------------------------------------
💡 ROBUSTNESS IMPROVEMENTS:
-
Enhanced deep-link validation
• Error handling enhancements: Check whether your TWA receives unexpected deep-link paths (like /anything-other-than-copy-trading) and handle them gracefully.
• Input validation additions: In your TWA or PWA code, confirm the path is valid before routing.
• State management improvements: If your TWA loads a root “/” path, ensure any session or preference logic is updated to reflect the new entry point.
• Code example for each suggestion:
// Inside your web app’s router logic (e.g., React Router or similar)
function handleRoute(path) {
const validPaths = ['/', '/some-other-valid-route'];
if (!validPaths.includes(path)) {
// Redirect or show a custom 404 logic
window.location.href = '/404';
}
}
------------------------------------------------------------------
-
Include optional fingerprint checking for additional security
• Error handling enhancements: If your domain changes often or you anticipate multiple subdomains, adding more fingerprints with a safe fallback or error path can prevent domain mismatch issues.
• Code example:
"fingerprints": [
{ "value": "40:F6:E7:17:8B:FB:E8:3C:9B:9F:83:AD:FC:0A:6C:D3:5B:40:92:6D:3D:BE:C2:86:02:C0:C7:99:A8:B9:20:23" },
{ "value": "<additional_fingerprint>" }
],
------------------------------------------------------------------
- Provide clear migration instructions for end users
• If your users bookmarked /copy-trading/, link them to the new root path to maintain continuity. This is more of a UX improvement but helps reduce confusion.
By addressing these issues—especially the path prefix removal in the Intent Filter and verifying HTTPS availability—you’ll ensure a smoother transition to the new domain while preserving the logical correctness and reliability of your TWA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @aum-deriv - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Build: