fix: dapp connect#116
Open
web3miaomiao wants to merge 2 commits intomainfrom
Open
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the Dexie migration for dapp connection history so that legacy Aleo connection records are correctly migrated into the new dapp_history table using the migration transaction instead of an instance field, and cleans up the deprecated aleo_history table definition. Updated class diagram for DappDatabase migration logicclassDiagram
class Dexie {
+version(number): Dexie
+stores(schema): Dexie
+upgrade(migration): Dexie
}
class DappDatabase {
+dapp_history: Dexie.Table~ConnectHistory,string~
+request: Dexie.Table~DappRequest,string~
+constructor()
}
class OldDappDatabase {
+aleo_history: Dexie.Table~AleoConnectHistory,string~
+dapp_history: Dexie.Table~ConnectHistory,string~
+request: Dexie.Table~DappRequest,string~
}
Dexie <|-- DappDatabase
Dexie <|-- OldDappDatabase
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the migration,
bulkAddwill throw if any record conflicts on the primary key; if this migration might run multiple times or on partially-migrated data, consider usingbulkPutor handling duplicates explicitly to make it idempotent. - Instead of using string-based table access (
tx.table("aleo_history")/tx.table("dapp_history")) with a type assertion, consider using the typed table definitions onthis(or a typed helper) to avoid theas AleoConnectHistory[]cast and keep the migration strongly typed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the migration, `bulkAdd` will throw if any record conflicts on the primary key; if this migration might run multiple times or on partially-migrated data, consider using `bulkPut` or handling duplicates explicitly to make it idempotent.
- Instead of using string-based table access (`tx.table("aleo_history")` / `tx.table("dapp_history")`) with a type assertion, consider using the typed table definitions on `this` (or a typed helper) to avoid the `as AleoConnectHistory[]` cast and keep the migration strongly typed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved the deprecated ChangesDatabase Migration Deprecation & Migration Flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)app/database/DappDatabase.tsMicrosoft Presidio Analyzer failed to scan this file Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/database/DappDatabase.ts (1)
25-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a forward-fix migration for already-migrated databases.
Version 3's upgrade hook only runs during the v2→v3 transition. Users already on v3 or v4 with pre-existing
dapp_historyrecords missingcoinTypewill not be backfilled. The index[address+coinType+network](line 27) requires this field on all records.Add a v5 migration to backfill missing
coinTypevalues:Suggested patch
this.version(4).stores({ aleo_history: null, aleo_connect_history: null, }); + + this.version(5).upgrade(async (tx) => { + const table = tx.table("dapp_history"); + const rows = (await table + .filter((row: Partial<ConnectHistory>) => !row.coinType) + .toArray()) as ConnectHistory[]; + + if (!rows.length) return; + + await table.bulkPut( + rows.map((row) => ({ + ...row, + coinType: CoinType.ALEO, + })), + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/database/DappDatabase.ts` around lines 25 - 43, Add a forward-fix migration (e.g., this.version(5).upgrade(...)) that scans the dapp_history table for records missing coinType and backfills them with CoinType.ALEO; specifically, in the new upgrade handler use tx.table("dapp_history").toArray(), filter entries where record.coinType === undefined || record.coinType === null, map each to {...record, coinType: CoinType.ALEO} and write them back with tx.table("dapp_history").bulkPut(...) (preserving the primary key) so the new index [address+coinType+network] won’t break for already-migrated v3/v4 databases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/database/DappDatabase.ts`:
- Around line 25-43: Add a forward-fix migration (e.g.,
this.version(5).upgrade(...)) that scans the dapp_history table for records
missing coinType and backfills them with CoinType.ALEO; specifically, in the new
upgrade handler use tx.table("dapp_history").toArray(), filter entries where
record.coinType === undefined || record.coinType === null, map each to
{...record, coinType: CoinType.ALEO} and write them back with
tx.table("dapp_history").bulkPut(...) (preserving the primary key) so the new
index [address+coinType+network] won’t break for already-migrated v3/v4
databases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d290f8af-a347-47b5-b3a2-f77cd348503a
📒 Files selected for processing (1)
app/database/DappDatabase.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修复了app升级时,老版本dapp连接历史数据库的bug(migration时没有赋值)
Summary by Sourcery
Fix migration of legacy dapp connection history when upgrading the local Dapp database.
Bug Fixes:
Enhancements:
Summary by CodeRabbit