Ux touchups - #10
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gaston-review[bot]](https://avatars.githubusercontent.com/in/3010035?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gaston Review
Verdict: Comment
Score: ██████░░░░ 6/10
Pull Request Summary
This PR is a large batch of changes: v1.3 contract migration (new factory and leaderboard addresses), a full rewrite of the BuyMessageModal (consolidating the deleted TopDawgModal), new features (account dashboard, embed pages, ETH price display, MARKEE token estimates, RevnetBuyWidget), CI/CD additions (PR checks, main-to-dev mergeback), CSP updates for embed iframing, and various UX polish across modals and the homepage.
Review Summary
🟡 Warning — MARKEE token estimate in BuyMessageModal has a suspicious extra 0.62 multiplier
BuyMessageModal calculates ethAmount * 0.38 * 0.62 * rate * 0.62. FixedPriceModal uses eth * rate * 0.62 and RevnetBuyWidget uses eth * rate * 0.62. For leaderboard purchases where 38% goes to the RevNet, the expected formula is eth * 0.38 * rate * 0.62. The extra * 0.62 would show users ~62% fewer tokens than they'll actually receive. If intentional (some RevNet-internal split), it needs a comment. If not, it's a user-facing bug in the purchase modal.
🟡 Warning — Three divergent MARKEE token phase/issuance implementations
BuyMessageModal and FixedPriceModal define identical PHASES arrays with hardcoded dates. RevnetBuyWidget uses a different buildPhases() function with different season math. These will inevitably diverge. Extract a single shared module for token estimation.
🔵 Suggestion — Significant duplicated code across modals
BtnTooltip, TxRing, design tokens (MONO, PINK, BLUE, BG, etc.), and PHASES/getCurrentPhaseRate are copy-pasted across BuyMessageModal, FixedPriceModal, and ExpandableMarkeeRow. This is a maintenance liability — when colors or phase schedules change, you'll need to update 3+ files.
🔵 Suggestion — API routes using fromBlock: 0n for log queries may be slow
The /api/account/funded route scans FundsAdded logs from block 0 across potentially many contracts. On Base with millions of blocks, this can be expensive and slow. Consider tracking a deployment block number or using a more targeted block range.
The direction is solid and there's a lot of nice UX work here, but I see a likely bug in the MARKEE token estimation formula for leaderboard purchases, a lot of duplicated code that will rot fast, and some API performance concerns. The token estimation bug is the one I'd want resolved before merging since it directly affects what users see before spending ETH.
📌 7 inline comments
🟡 Warning: 1 · 🔵 Suggestion: 5 · ⚪ Nitpick: 1
🔍 Reviewed by Gaston
|
|
There was a problem hiding this comment.
Gaston Review
Verdict: Comment
Score: ██████░░░░ 6/10
Pull Request Summary
This PR is a large UX polish pass: unified ExpandableMarkeeRow component with on-chain transaction history, ConnectButton dropdown menu replacing the separate account link, Privy wallet fallback for embedded wallets, dirty-form protection (blocking backdrop/Escape close), "Use max" balance buttons, consistent transaction error formatting, gas reserve constants, escape key support across all modals, displayTotalFunds consistency fix (max of totalFunds vs summed topFunds), CI for typechecking, and a main→dev mergeback workflow.
Review Summary
🟡 Warning — MARKEE token estimate formula still has extra 0.62 multiplier
BuyMessageModal line 47: ethAmount * 0.38 * 0.62 * getCurrentPhaseRate() * 0.62. FixedPriceModal uses eth * rate * 0.62. The extra * 0.62 in BuyMessageModal understates the token estimate by ~38% for leaderboard purchases. This was flagged in the prior review and remains unaddressed.
🟡 Warning — fromBlock: 0n proliferating — now in 10+ call sites
The new /api/markee/history route adds 5 more fromBlock: 0n log scans, and ExpandableMarkeeRow adds 3 more client-side. On Base with millions of blocks, this is expensive. A shared deployment block constant would fix all of them at once.
🔵 Suggestion — PHASES and getCurrentPhaseRate still duplicated across modals
BuyMessageModal and FixedPriceModal both define identical PHASES arrays and getCurrentPhaseRate functions. A single shared module would prevent these from drifting apart.
The latest push is just a merge of main into the branch — none of my prior review comments have been addressed. The suspicious MARKEE token formula (extra 0.62 multiplier) is still there, PHASES are still duplicated, and fromBlock: 0n is now used in even more places (new API route + ExpandableMarkeeRow). The new code is well-structured and the UX improvements are solid, but the token estimate bug should be resolved before merging.
📌 5 inline comments
🔵 Suggestion: 2 · ⚪ Nitpick: 3
🔍 Reviewed by Gaston
|
|
||
| const key = markee.address.toLowerCase() | ||
| if (sessionTracked.has(key)) return | ||
| if (!markee.message) return |
There was a problem hiding this comment.
⚪ Nitpick: Still unresolved: duplicate if (!markee.message) return guard — line 51 already checks this, so the check on line 55 is dead code.
| address: leaderboard, | ||
| event: LEADERBOARD_MARKEE_CREATED, | ||
| args: { markeeAddress: markee }, | ||
| fromBlock: 0n, |
There was a problem hiding this comment.
🔵 Suggestion: Five log queries all using fromBlock: 0n. Since these are scoped to a specific leaderboard+markee address pair, the blast radius is smaller than the account/funded route, but it's still scanning from genesis. Consider a shared DEPLOYMENT_BLOCK constant (e.g. the earliest factory deployment block on Base) that all log queries reference.
| }, [markee.address]) // eslint-disable-line react-hooks/exhaustive-deps | ||
|
|
||
| useEffect(() => { | ||
| if (!expanded || history.length > 0) return |
There was a problem hiding this comment.
🔵 Suggestion: history.length > 0 means once loaded, history is never refreshed until the component remounts. If the user adds funds and re-expands, they'll see stale data. Consider either resetting history when the row collapses, or adding a 'refresh' button in the expanded panel.
| @@ -21,6 +22,7 @@ const BLUE = '#7C9CFF' | |||
| const BORDER = 'rgba(138,143,191,0.2)' | |||
| const MUTED = '#8A8FBF' | |||
| const TEXT = '#EDEEFF' | |||
There was a problem hiding this comment.
⚪ Nitpick: FAST_TX_GAS_RESERVE is defined as a raw bigint literal 200000000000000n here, but as parseEther('0.0002') in BuyMessageModal. Same value, different representations. Pick one style.
| const TEXT2 = '#B8B6D9' | ||
| const MUTED = '#8A8FBF' | ||
| const BORDER = 'rgba(138,143,191,0.2)' | ||
| const LB_COLS = '42px 150px 120px minmax(260px,1fr) 70px 170px' |
There was a problem hiding this comment.
⚪ Nitpick: Design tokens (MONO, PINK, BLUE, BG, BG2, TEXT, TEXT2, MUTED, BORDER) are copy-pasted here and in at least 3 other files. A shared lib/theme.ts would prevent drift.
|
|
|
|
There was a problem hiding this comment.
Gaston Review
Verdict: Comment
Score: ███████░░░ 7/10
Pull Request Summary
This PR is a substantial UX polish pass across the Markee frontend: it consolidates duplicate MarkeeRow components into a shared ExpandableMarkeeRow with inline transaction history, extracts duplicated token phase calculations into lib/tokenPhases.ts, adds a transaction error formatter, improves wallet detection using Privy's useWallets alongside wagmi, adds escape key handling and dirty-form protection to modals, replaces the header account link with a dropdown menu in ConnectButton, tightens CSP frame-ancestors, adds CI/CD workflows, and fixes several display issues around total funds calculations.
Review Summary
🟡 Warning — Transaction error formatter swallows useful error messages
The NOISY_TRANSACTION_PATTERNS list in transactionErrors.ts includes very broad substrings like 'abi', 'decode', 'function:', and 'details:'. These are checked against the combined shortMessage + full message. Since viem contract errors always contain ABI data in their verbose .message, this means every viem contract error will match and return the generic "Transaction error" — even when viem's shortMessage contains a clean, useful explanation of what went wrong. Users will see "Transaction error" for everything from insufficient funds to contract reverts, with no way to tell what happened. The fix is straightforward: when the full message is noisy but shortMessage exists, return shortMessage instead of the generic fallback.
🔵 Suggestion — History fetching has no limits or caching
The new /api/markee/history route and the client-side fallback in ExpandableMarkeeRow both fetch all logs from BASE_MARKEE_EVENTS_FROM_BLOCK to latest with no pagination. For a popular markee, this could return thousands of events and trigger many block-fetch RPC calls for timestamps. The effect dependencies also cause re-fetches whenever parent data changes, which compounds the issue after the new refreshAfterTransaction calls.
Solid, well-scoped UX improvement PR. The consolidation of duplicated code (token phases, markee rows, error formatting) is the right call. The one thing I'd want fixed before merge is the transaction error formatter — as written, it will show "Transaction error" for every contract interaction failure, hiding useful information from users. That's a quick fix. The rest is in good shape.
📌 7 inline comments
🟡 Warning: 2 · 🔵 Suggestion: 4 · ⚪ Nitpick: 1
🔍 Reviewed by Gaston
| 'docs:', | ||
| 'details:', | ||
| 'version:', | ||
| 'abi', |
There was a problem hiding this comment.
🟡 Warning: The pattern 'abi' is a 3-character substring match against the full error message (combined = shortMessage + raw message). This will match words like "ability", "capability", "available", etc. More importantly: viem's ContractFunctionExecutionError always includes ABI data in its full .message, so the check on line 58 will match every single viem contract error and return the generic "Transaction error" — even when viem provides a perfectly clean shortMessage like "Insufficient funds" or "Reverted: Not enough ETH".
The fix is to check noisy patterns only against raw (not combined), and if matched, return short (the shortMessage) when it exists instead of the generic fallback. Something like:
if (short && matchesAny(raw, NOISY_TRANSACTION_PATTERNS)) return short
if (matchesAny(combined, NOISY_TRANSACTION_PATTERNS)) return 'Transaction error'Similarly, 'decode', 'function:', and 'details:' are broad enough to match legitimate error text.
|
|
||
| fetchHistory() | ||
| return () => { cancelled = true } | ||
| }, [expanded, leaderboardAddress, markee.address, markee.message, markee.name, markee.totalFundsAdded, publicClient]) |
There was a problem hiding this comment.
🔵 Suggestion: This dependency array includes markee.message, markee.name, and markee.totalFundsAdded. If the parent refetches leaderboard data (which the markee detail page now does via refreshAfterTransaction), every expanded row will re-fetch its entire history from the chain/API. History is append-only — it won't change between fetches. Consider removing the markee data fields from deps and only re-fetching on explicit user action (e.g. a refresh button), or cache the result keyed by markee address.
| @@ -424,14 +456,14 @@ export function BuyMessageModal({ | |||
| </div> | |||
There was a problem hiding this comment.
🔵 Suggestion: When the wallet is connected via Privy but wagmi hasn't hydrated yet (hasWallet is true via wallets[0], but hasActiveWalletConnection is false because isConnected is false), the modal shows the "Connect your wallet" screen. The condition !hasWallet || !hasActiveWalletConnection means a user who IS logged in via Privy will briefly see a connect prompt during hydration. Consider gating this on mounted or a loading state to avoid a flash.
| const allLogs = [...createdLogs, ...migratedLogs, ...fundsLogs, ...messageLogs, ...nameLogs] | ||
| const blockNumbers = allLogs.map(log => log.blockNumber).filter((n): n is bigint => n !== null) | ||
| const uniqueBlocks = [...new Set(blockNumbers.map(String))].map(BigInt) | ||
| const blocks = await Promise.all(uniqueBlocks.map(blockNumber => client.getBlock({ blockNumber }))) |
There was a problem hiding this comment.
🔵 Suggestion: Fetching blocks individually with Promise.all(uniqueBlocks.map(...)) could fire dozens of RPC calls for a markee with a long history. Consider using eth_getBlockByNumber in batches or adding a reasonable limit to the number of events returned (e.g., last 50). The 5 parallel getLogs calls also have no pagination — for a popular markee, this could return thousands of logs and hit RPC limits.
| if (matchesAny(combined, USER_REJECTED_PATTERNS)) { | ||
| return 'Transaction rejected' | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Warning: When the noisy pattern matches, the function returns 'Transaction error' even if shortMessage exists with a useful, clean message. Viem errors almost always have a shortMessage that's human-readable (e.g., "The contract function reverted"). Returning the generic string here means users never see what actually went wrong on-chain. This will make debugging transaction failures very frustrating.
| const TEXT2 = '#B8B6D9' | ||
| const MUTED = '#8A8FBF' | ||
| const BORDER = 'rgba(138,143,191,0.2)' | ||
| const LB_COLS = '42px 150px 120px minmax(260px,1fr) 70px 170px' |
There was a problem hiding this comment.
⚪ Nitpick: This LB_COLS constant is duplicated as MARKEE_LB_COLS in markee/[address]/page.tsx. If either changes without the other, the column headers and row columns will misalign. Consider exporting it from the component or a shared constants file.
|
|
||
| // Embed pages need frame-ancestors: * so third-party sites can iframe them. | ||
| const embedCspHeader = Object.entries({ ...cspDirectives, 'frame-ancestors': ['*'] }) | ||
| // Embed pages can be framed by HTTPS third-party sites. |
There was a problem hiding this comment.
🔵 Suggestion: Tightening frame-ancestors from * to https: is a good security move. Just be aware this will break any embed integrations running on plain HTTP (e.g., local dev servers or HTTP-only sites embedding markee widgets). If that's expected, this is fine.
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: ████████░░ 8/10
Pull Request Summary
This PR addresses all the critical and most non-critical feedback from my previous two reviews: the MARKEE token formula bug is fixed (extra 0.62 removed), PHASES/getCurrentPhaseRate are extracted into a shared lib/tokenPhases.ts, fromBlock: 0n is replaced everywhere with BASE_MARKEE_EVENTS_FROM_BLOCK, and the CSP is tightened from frame-ancestors: * to https:. On top of the fixes, it adds a unified ExpandableMarkeeRow with on-chain transaction history, a ConnectButton dropdown menu, Privy embedded wallet fallback, dirty-form protection (blocking backdrop/Escape close), "Use max" balance buttons, consistent transaction error formatting via a new lib/transactionErrors.ts, a gas reserve constant, CI typechecking, and a main→dev mergeback workflow.
Review Summary
🔵 Suggestion — displayTotalFunds logic is copy-pasted across 5 locations
The max(totalFunds, summedTopFunds) pattern appears identically in the GitHub, Superfluid, and Website leaderboard pages, plus the GitHub leaderboards API and usePartnerMarkees hook. A shared utility like getDisplayTotalFunds(totalFunds, topFunds) would keep these in sync — right now a change to the edge case logic requires updating all five spots.
🔵 Suggestion — Escape key handler pattern duplicated across 8 files
The useEffect(() => { const handleEscape = ... window.addEventListener('keydown', handleEscape); return () => ... }) pattern is repeated in every modal. A small useEscapeKey(isOpen, onClose, blocked?) hook would remove ~10 lines from each modal and centralize the dirty-form-blocking behavior.
Clean response to review feedback. All blockers from my prior reviews are resolved — the token estimate formula now matches the documented split, phases are properly shared, log queries use a deployment block, and the CSP is reasonably tightened. The new features are well-structured. The only things left to flag are minor deduplication opportunities that won't block a merge.
📌 4 inline comments
🔵 Suggestion: 1 · ⚪ Nitpick: 3
🔍 Reviewed by Gaston
| return ethAmount * getCurrentGrossMarkeeRate(now) * REVNET_BUYER_TOKEN_SHARE | ||
| } | ||
|
|
||
| export function estimateLeaderboardPurchaseMarkeeTokens(ethAmount: number, now = Date.now()): number { |
There was a problem hiding this comment.
⚪ Nitpick: Nice — eth * 0.38 * rate * 0.62 is exactly the formula I expected. The old eth * 0.38 * 0.62 * rate * 0.62 was underselling by ~38%. This is correct now.
| })), | ||
| ...messageLogs.map(log => ({ | ||
| id: `${log.transactionHash}-${log.logIndex}`, | ||
| kind: 'message' as const, |
There was a problem hiding this comment.
🔵 Suggestion: The early return guard on line 256 (if (!leaderboardAddress && !publicClient) return) means we only skip fetch when BOTH are missing. When leaderboardAddress is falsy but publicClient is truthy, execution falls to the client-side RPC path. The publicClient! non-null assertions on lines 302-304 are safe given this logic, but the guard condition reads like it's checking "either is missing" rather than "both are missing." A comment or restructuring into two separate branches (API path vs client path) with their own individual guards would make this less surprising to future readers.
| return ( | ||
| <span | ||
| style={{ position: 'relative', display: 'inline-flex' }} | ||
| onMouseEnter={() => setOpen(true)} |
There was a problem hiding this comment.
⚪ Nitpick: The wallets[0]?.address fallback is a nice Privy embedded wallet fix. Worth noting that wallets from useWallets() can change order depending on which wallet the user has active — if you ever need to pick a specific wallet type (embedded vs external), you may want to filter by wallet.walletClientType instead of taking index 0.
|
|
||
| // Embed pages need frame-ancestors: * so third-party sites can iframe them. | ||
| const embedCspHeader = Object.entries({ ...cspDirectives, 'frame-ancestors': ['*'] }) | ||
| // Embed pages can be framed by HTTPS third-party sites. |
There was a problem hiding this comment.
⚪ Nitpick: frame-ancestors: https: is a good middle ground. If the set of embedding partners is ever known and finite, narrowing to an explicit allowlist would be even better, but this is a solid improvement over *.
1 participant
No description provided.