-
Notifications
You must be signed in to change notification settings - Fork 53
Add transaction details navigation from push notifications #1288
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
base: main
Are you sure you want to change the base?
Add transaction details navigation from push notifications #1288
Conversation
Summary of ChangesHello @DRadmir, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience by enabling direct navigation to transaction details from push notifications. It involves the introduction of a new API endpoint for fetching individual transactions, updates to core services for handling these fetches, and modifications to the push notification parsing and handling logic to support this deep-linking functionality. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request implements navigation to transaction details from push notifications. The changes involve updating the API definitions, services, and UI to handle the new notification payload and fetch the required transaction data. My review focuses on a critical race condition when switching wallets, opportunities to remove redundant data fetching for performance, ensuring data consistency when storing fetched transactions, and a minor code simplification. Overall, the changes are well-structured, but the identified issues should be addressed to ensure correctness and performance.
Packages/FeatureServices/TransactionsService/TransactionsService.swift
Outdated
Show resolved
Hide resolved
- Create TransactionLoaderViewModel and TransactionLoaderView for loading transaction details - Add assetId parameter to Scenes.Transaction for proper asset fetching - Implement getTransaction API endpoint (GET /v1/transactions/{transactionId}) - Add getTransaction method to TransactionsService with database storage - Update TransactionLoaderViewModel to fetch assets and transactions asynchronously - Use observeQuery and task for reactive transaction loading - Update push notification handler to navigate to transaction details view
81a5a4a
to
fa68b2b
Compare
…-transaction-details-view-when-press-on-transaction
Gem/Views/MainTabView.swift
Outdated
if let walletId = walletService.currentWalletId { | ||
let transaction = try await transactionsService.getTransaction(walletId: walletId, transactionId: transactionId) | ||
|
||
navigationState.wallet.append(Scenes.Asset(asset: transaction.asset)) |
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.
do wallet = []
if possible to avoid having too many paths in the stack
if walletIndex != model.wallet.index.asInt { | ||
walletService.setCurrent(for: walletIndex) | ||
} | ||
|
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.
I wonder if we should push navigationState.wallet.append(Scenes.Asset(asset: transaction.asset))
first and then do preload transaction next, in this case if transactionsService.getTransaction
is slow, you are at right asset. this works only if you have an asset already
…-transaction-details-view-when-press-on-transaction
f9e0b35
to
fe8acb5
Compare
Summary
Implements transaction details navigation from push notifications. When a user taps on a transaction notification, the app fetches the transaction details and navigates directly to the transaction view.
Changes
GET /v1/transactions/{transactionId}
API endpointgetTransaction
method inTransactionsService
with database storageTransactionLoaderViewModel
andTransactionLoaderView
for loading transaction detailsScenes.Transaction
to includeassetId
parameterTODO