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

sagar / Refactor android config #78

Merged
merged 2 commits into from
Jan 10, 2025
Merged

Conversation

aum-deriv
Copy link
Collaborator

@aum-deriv aum-deriv commented Jan 10, 2025

Summary by Sourcery

Build:

  • Update app version to 9 and change the host name to copy-trading.binary.sx.

Copy link

vercel bot commented Jan 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
copy-trading ✅ Ready (Inspect) Visit Preview Jan 10, 2025 0:12am

Copy link

sourcery-ai bot commented Jan 10, 2025

Reviewer's Guide by Sourcery

This 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 changes

graph 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]
Loading

File-Level Changes

Change Details Files
Updated app configuration twa-build/app/build.gradle
Removed redundant path prefix
  • Removed path prefix '/copy-trading/' from intent filter data tag
twa-build/app/src/main/AndroidManifest.xml
Updated site URL in strings.xml twa-build/app/src/main/res/values/strings.xml
Updated web app manifest and checksum
  • Updated web app manifest file
  • Regenerated manifest checksum file
  • Updated TWA manifest file
twa-build/app/src/main/res/raw/web_app_manifest.json
twa-build/manifest-checksum.txt
twa-build/twa-manifest.json

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Collaborator

@review-deriv review-deriv left a comment

Choose a reason for hiding this comment

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

AI Code Review

⚠️ IMPORTANT: This review is AI-generated and should be validated by human reviewers
🔴 BUGS & LOGICAL ISSUES:

  1. 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:

    ------------------------------------------------------------------
  2. 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.

  3. 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:

  1. 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.

  2. 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:

  1. 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';
}
}
------------------------------------------------------------------

  1. 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>" }
],
------------------------------------------------------------------

  1. 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.

Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@aum-deriv aum-deriv merged commit 32f8968 into deriv-com:main Jan 10, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants