Conversation
GigaHierz
left a comment
GigaHierz
left a comment
There was a problem hiding this comment.
head 98f9234 · fetch matched headRefOid (no STALE); branch 7 commits behind main, merge-tree clean, reviewed on a merge of head into current main; mint broken-links --check-redirects pass, check-orphans.sh pass, CI green (broken-links, GitGuardian, StepSecurity); mint dev: 0 console errors on /tooling/wallets/metamask/use, /build-on-celo/network-overview, /home/wallets at 1440x900 and 412x915, both add-network buttons render on network-overview (372x134 at 412 wide, no horizontal scroll)
Verdict: REQUEST-CHANGES
Scope: one file, tooling/wallets/metamask/use.mdx, 2 lines changed (an end-user bullet and the closing Note link). The PR does not touch network-overview, the button snippet or docs.json.
Findings
-
LOW-MEDIUM · tooling/wallets/metamask/use.mdx:18 · The bullet "Add Celo Mainnet or Celo Sepolia to MetaMask" promises an action but links to a developer reference page, with the button below the fold, while the real end-user walkthrough is not linked.
- Scenario: an end user clicks it and lands on "Network Information", a chain-parameter table page whose first screen is the mainnet table and a "Replace Alfajores" Sepolia blurb. The mainnet button sits about 966px down at 1440x900 and about 1164px down at 412x915. The page has no MetaMask instructions and no mention of what the button does. The existing step-by-step page
/tooling/wallets/metamask/add-celo-testnet-to-metamask(in nav under the same MetaMask group) is not linked, and main'shome/walletsalready links it. - Fix shape: keep the network-overview link but make the label match what it is, e.g. "Add Celo Mainnet to MetaMask with one click (network information)". Add
[Add Celo Sepolia manually](/tooling/wallets/metamask/add-celo-testnet-to-metamask)as the testnet end-user path. Do not restate chain parameters here (one fact, one page).
- Scenario: an end user clicks it and lands on "Network Information", a chain-parameter table page whose first screen is the mainnet table and a "Replace Alfajores" Sepolia blurb. The mainnet button sits about 966px down at 1440x900 and about 1164px down at 412x915. The page has no MetaMask instructions and no mention of what the button does. The existing step-by-step page
-
LOW · tooling/wallets/metamask/use.mdx:47 · The link text "here" on the edited line carries no information for readers or agents.
- Scenario: an agent traversing links sees
[here](/home/wallets)and cannot tell it is the list of Celo native wallets. - Fix shape:
[the native wallets on Wallets for users](/home/wallets#celo-native-wallets)(anchor exists on main: "## Celo native wallets").
- Scenario: an agent traversing links sees
-
LOW · tooling/wallets/metamask/use.mdx:14,22,26,30,38,42 (page edited by this PR) · AGENTS.md section 3 says headings on pages you are already editing get fixed. The page still has bold, Title Case headings ("How to use MetaMask with Celo", "Things to Keep in Mind", "Private Key Import", "Gas Fees Require CELO", "Incorrect Logo"). It has no
## Related, no first-paragraph audience sentence, a<Danger>block before the intro, and a non-outcomedescription("Overview of MetaMask and how you can get started...").- Scenario: the page goes through a review again and the next docs PR has to touch the same lines.
- Fix shape: sentence-case the headings without
**, add## Relatedlinking/tooling/wallets/metamask/setup,/home/wallets,/build-on-celo/network-overview, rewritedescriptionas an outcome. Alternatively file an issue and link it in the PR body.
-
LOW (PR body) · The body says #2349 and #2350 add a
metamask/setuplink for holders onhome/wallets.mdxand that "whichever merges should use network-overview". Both PRs are merged (2026-10-02). Main'shome/wallets.mdxhas nometamask/setuplink; its MetaMask guide bullet already links network-overview and add-celo-testnet-to-metamask. The sentence is stale and misleads the next reader.- Fix shape: delete that sentence from the body.
Verified good
- Head SHA matches the API; merge with main is clean; the PR file is unchanged on main (use.mdx on main still has both old links).
/build-on-celo/network-overviewis in docs.json navigation (line 174 on main)./home/walletsexists and is reachable./tooling/wallets/ledger/setupand/tooling/wallets/metamask/setupare in nav. The links resolve when clicked in mint dev (hrefs checked in the rendered DOM). Neither new link uses an anchor, so no anchor to break.- Claim "metamask/setup is the developer page": true. It is "Programmatic Setup", titled "How app developers can use MetaMask", and shows
wallet_addEthereumChainsnippets. The developer bullet to it is kept. - Claim "add-network buttons for Mainnet and Sepolia live on network-overview": true.
<AddNetworkButton network="mainnet" />andnetwork="sepolia"at lines 25 and 42, imported at line 6. Props match the component'sNETWORKSkeys. Rendered labels "Add Celo Mainnet to your wallet" (Chain ID 42220) and "Add Celo Sepolia to your wallet" (Chain ID 11142220). Clicking with no wallet shows "No browser wallet detected. Install MetaMask, then try again." and the console stays clean. - Claim "the closing note promised a list of Celo native wallets": true.
/home/walletshas a "Celo native wallets" section (MiniPay, Valora, Celo Terminal); the old target was the developer page. - Added text has no promotional words, no rocket emoji, no Alfajores, no blockquote callouts. The Note stays a
<Note>. mint broken-links --check-redirectsandscripts/check-orphans.shpass on the merged result. (Anchors are not checked by broken-links; the PR adds none.)
What the PR got right
- It correctly spots that an end-user list pointed at a developer page, and that the Note promised a wallet list the target did not have. Both are real misdirections, and the fix is minimal and correctly scoped to one file.
- It reuses the existing single-source button instead of restating chain parameters, which respects one fact one page.
- It leaves the developer bullet in place and says so in the body.
Remaining work is one round of small text fixes (findings 1 and 2, plus 3 or a filed issue and 4), not a rethink.
Merge-order hazards
gh pr list over open PRs: none touch tooling/wallets/metamask/use.mdx, home/wallets.mdx or build-on-celo/network-overview.mdx. #2364 and #2346 touch docs.json but this PR does not, so no conflict. No stack or signature hazards.
Screenshots: /tmp/reviews/shots/2355-{use,netover,wallets}-{desktop,mobile}.png and 2355-netover-fold.png.
Link the Sepolia manual guide, give the wallets link a descriptive label, sentence-case the headings, add an audience line and Related section.
98f9234 to
920d8a3
Compare
|
All four are in 920d8a3, rebased onto current main.
|
…uides that don't exist
|
One more commit, 8a75393, for wording on the same page that the review did not flag. It removes "we're excited to bring its functionality to the Celo ecosystem" and rewrites the Donut hard fork sentence so it no longer says "now supports". It also deletes "See these guides if you accidentally sent ETH to CELO addresses", which pointed to guides that do not exist. |
This branch has not been deployed
2 participants
The MetaMask overview sent end users to
/tooling/wallets/metamask/setup, the developer page for callingwallet_addEthereumChainfrom an app, and its closing note linked the same page while promising a list of Celo native wallets. This sends end users to the add-network buttons on the network overview and to the manual Celo Sepolia guide, and points the note at the native wallets section of/home/wallets.home/wallets.mdxalready links those two pages the same way. The developer link to the setup page stays.Since the PR edits the page anyway, it also applies the AGENTS.md section 3 rules. Headings are sentence case without bold, the first paragraph says who the page is for, the intro comes before the
<Danger>block, thedescriptionsays what the reader can do, and the page ends with a## Relatedlist. No page links to an anchor on this one, so no other files changed.mint broken-links: success no broken links found.scripts/check-orphans.sh: No orphan pages found.