-
Notifications
You must be signed in to change notification settings - Fork 471
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
Fix(Tx notes): fire analytics on tx submit + adjust design #4771
Conversation
Branch preview✅ Deploy successful! Storybook: |
📦 Next.js Bundle Analysis for @safe-global/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
Page | Size (compressed) |
---|---|
global |
1.04 MB (🟡 +54.13 KB) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script>
tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
Thirty-one Pages Changed Size
The following pages changed size from the code in this PR compared to its base branch:
Page | Size (compressed) | First Load |
---|---|---|
/ |
508 B (🟢 -2 B) |
1.04 MB |
/address-book |
23.21 KB (🟡 +142 B) |
1.06 MB |
/apps |
35.8 KB (🟡 +2.08 KB) |
1.08 MB |
/apps/custom |
33.87 KB (🟡 +2.08 KB) |
1.07 MB |
/apps/open |
55.57 KB (🟡 +1.96 KB) |
1.1 MB |
/balances |
29.88 KB (🟡 +176 B) |
1.07 MB |
/balances/nfts |
9.52 KB (🟢 -24 B) |
1.05 MB |
/bridge |
2.56 KB (🟡 +2 B) |
1.04 MB |
/cookie |
8.77 KB (🟡 +1 B) |
1.05 MB |
/home |
61.38 KB (🟡 +2.12 KB) |
1.1 MB |
/new-safe/advanced-create |
26.38 KB (🟢 -70 B) |
1.07 MB |
/new-safe/create |
25.52 KB (🟢 -71 B) |
1.07 MB |
/privacy |
14.57 KB (🟡 +2 B) |
1.06 MB |
/settings/appearance |
2.25 KB (🟡 +2 B) |
1.04 MB |
/settings/environment-variables |
3.27 KB (🟢 -1 B) |
1.04 MB |
/settings/modules |
4.06 KB (🟡 +1 B) |
1.05 MB |
/settings/notifications |
6.33 KB (🟢 -14.99 KB) |
1.05 MB |
/settings/safe-apps |
20.35 KB (🟡 +2.08 KB) |
1.06 MB |
/settings/security |
2.34 KB (🟡 +1 B) |
1.04 MB |
/settings/setup |
30.83 KB (🟡 +94 B) |
1.07 MB |
/share/safe-app |
7.56 KB (🟢 -5 B) |
1.05 MB |
/stake |
618 B (🟢 -1 B) |
1.04 MB |
/swap |
761 B (🟡 +1 B) |
1.04 MB |
/terms |
12.93 KB (🟡 +1 B) |
1.05 MB |
/transactions |
99.46 KB (🟡 +2.89 KB) |
1.14 MB |
/transactions/history |
99.42 KB (🟡 +2.89 KB) |
1.14 MB |
/transactions/messages |
60.25 KB (🟡 +1.95 KB) |
1.1 MB |
/transactions/msg |
56.5 KB (🟡 +1.95 KB) |
1.1 MB |
/transactions/queue |
49.36 KB (🟡 +1.96 KB) |
1.09 MB |
/transactions/tx |
48.72 KB (🟡 +1.96 KB) |
1.09 MB |
/welcome/accounts |
409 B (🟡 +2 B) |
1.04 MB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link
is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script>
tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🟡 | Statements | 74.25% (-0.03% 🔻) |
14760/19878 |
🔴 | Branches | 52.01% (-0.03% 🔻) |
3535/6797 |
🔴 | Functions | 57.35% (-0.03% 🔻) |
2095/3653 |
🟡 | Lines | 75.8% (-0.02% 🔻) |
13394/17671 |
Show files with reduced coverage 🔻
St.❔ |
File | Statements | Branches | Functions | Lines |
---|---|---|---|---|---|
🟢 | ... / SignOrExecuteForm.tsx |
89.41% (-2.15% 🔻) |
85.53% (-1.14% 🔻) |
50% | 88.89% (-2.25% 🔻) |
🟡 | ... / index.tsx |
72.73% (-14.77% 🔻) |
100% | 0% (-100% 🔻) |
77.78% (-22.22% 🔻) |
🟡 | ... / TxNoteForm.tsx |
57.14% (-42.86% 🔻) |
0% | 0% | 66.67% (-33.33% 🔻) |
🔴 | ... / TxNoteInput.tsx |
40% (-6.15% 🔻) |
100% | 0% | 44.44% (-5.56% 🔻) |
Test suite run success
1793 tests passing in 242 suites.
Report generated by 🧪jest coverage report action from aaed469
0f6c5dc
to
3d0e095
Compare
export function trackAddNote() { | ||
trackEvent(MODALS_EVENTS.ADD_TX_NOTE) | ||
} |
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.
nit: Not sure if we need this abstraction. Everywhere else we use trackEvent directly.
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 want to minimize what we import in SignOrExecute, plus encapsulate everything related to this feature in that folder.
if (customOrigin !== props.origin) { | ||
trackAddNote() | ||
} |
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.
What happens if the user didn't add a note and its not a safe apps transaction. Are both values undefined and we would try to track something? Actually no we are checking for inequality so this shouldn't be an issue
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.
Just trying to understand why we initialize customOrigin
with props.origin
instead of just leaving it undefined and if it exists we track an event.
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.
undefined equals undefined though.
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.
Ah, simply to avoid doing customOrigin || props.origin
when passing it down.
Tested the event changes. The event only triggers when a tx is submitted. Also verified the fix for the empty block you can see during tx execution when no tx note was created during tx proposal LGTM |
967d3f4
to
aaed469
Compare
What it solves
A follow-up on #4693.
Triggering the
Add tx note
on input change is firing too many times, so I'm changing it to fire only when you submit a tx with a note.Also, we've adjusted the design as per internal feedback:
How this PR fixes it
Submit tx note
to differentiate it from the previous, incorrectly tracked, eventonSubmit
prop name toonChange
for clarityorigin
limit better + align with the size of the input.