Migrate to Vite/React 19 and polish protocol table UX#15
Conversation
- migrate from CRA to Vite and update app entry/build setup - upgrade React/Ant Design and refresh dependency lockfile - add CI workflow and protocol icon coverage check script - centralize protocol/network/client display metadata and formatting helpers - add tests for data parsing, protocol display formatting, and icon mapping - add missing protocol icons (including Unichain) and protocol badge fallback - fix theme toggle button regression and improve table/search UX - enforce network/node/client ordering and hide rows with missing size data - normalize protocol/network naming (for example BNB Smart Chain, Polygon PoS) - refresh README and AGENTS guidance for the new toolchain
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughMigrates the project from Create React App to Vite, converts key components from .js to .jsx, adds protocol metadata/display helpers, introduces an icon-coverage validation script and CI workflow, updates docs and HTML metadata, and adds tests and styling updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 26-30: The workflow is missing a test step so CI doesn't run the
test suite; add a step that runs "npm run test" (or the project's test script)
into the job steps — e.g. insert a step named "Run tests" calling `npm run test`
(or `npm ci && npm test` if installing deps is needed) between the "Check
protocol icon coverage" and "Build app" steps (or before "Build app") so
failures fail the workflow; ensure the step uses the same runner/context as the
existing "Check protocol icon coverage" and "Build app" steps.
In `@index.html`:
- Line 28: The meta tags using relative paths (meta property="og:image" and meta
name="twitter:image") must use fully qualified absolute URLs; update the content
attributes for the elements identified by meta property="og:image" and meta
name="twitter:image" to an absolute URL (e.g.,
https://yourdomain.com/og-image.png), or construct them dynamically from a
canonical site base (env var or config) so social crawlers can resolve the
images correctly.
In `@package.json`:
- Around line 8-10: The three test-only packages "@testing-library/jest-dom",
"@testing-library/react", and "@testing-library/user-event" are currently listed
under dependencies in package.json; remove these three entries from the
"dependencies" section and add them under "devDependencies" with the same
version strings, then update the lockfile by running your package manager
(npm/yarn/pnpm install) so production installs no longer include them. Ensure
you modify package.json (look for the "dependencies" and "devDependencies" keys)
and run the install command to persist changes to package-lock.json/yarn.lock.
In `@scripts/check-protocol-icons.js`:
- Around line 148-151: The log uses error.message in the main().catch handler
which can contain newline characters and allow log injection; change the handler
in scripts/check-protocol-icons.js so you sanitize the error message before
printing (e.g., compute a safeMessage = (error && error.message) ?
String(error.message).replace(/[\r\n]+/g, ' ') : String(error) or
JSON.stringify(error) and then log that safeMessage in the console.error call),
keep process.exitCode = 1 as-is.
In `@src/App.jsx`:
- Around line 42-53: The filter in the useEffect uses
alias.includes(searchValue) which is case-sensitive while searchValue is
lowercased, so update the comparison to be case-insensitive by lowercasing
aliases before comparing (e.g., in the useEffect where setSearchResult is
called, call getProtocolSearchAliases(item.protocol).some(alias =>
alias.toLowerCase().includes(searchValue))), or alternatively ensure
getProtocolSearchAliases/formatProtocolDisplayName returns lowercased aliases;
change either getProtocolSearchAliases or the filter in useEffect to normalize
aliases with .toLowerCase() before .includes().
In `@src/components/ProtocolIcon/ProtocolIcon.jsx`:
- Around line 85-91: Guard against protocolName being null/undefined in
ProtocolIcon before calling split: ensure protocolName has a safe default (e.g.,
empty string) or return early. Update ProtocolIcon (where fallbackLabel is
computed) to coerce protocolName to a string (protocolName || '') or add a
default parameter in the ProtocolIcon signature so fallbackLabel calculation
never calls split on undefined/null. Keep the rest of the fallbackLabel logic
unchanged.
In `@src/components/TableWrapper/TableWrapper.jsx`:
- Around line 95-99: The merged-row lag happens because visibleData is updated
via setVisibleData in an effect/onChange path, so protocolRowSpans is computed
from stale visibleData; instead lift the sort/filter parameters into state
(e.g., sortState/filterState saved in the TableWrapper component when onChange
fires) and remove the effect that syncs visibleData, then derive visibleData
inside a useMemo from tableData + those params and compute protocolRowSpans from
that same memoized visibleData (so TableWrapper, visibleData, protocolRowSpans,
setVisibleData/onChange and useEffect are replaced by memoized derivation using
useMemo to keep both visibleData and protocolRowSpans consistent in the same
render).
🧹 Nitpick comments (14)
src/components/Header/Header.scss (1)
44-48: Consider adding a:focus-visibleoutline for keyboard accessibility.Since the button strips all default visual cues (border, shadow, background), keyboard users won't see a focus indicator. A subtle outline on
:focus-visiblewould pair well with thearia-labeladded in the JSX.♻️ Suggested tweak
&:hover, &:focus { background: transparent; color: var(--color-typo-primary); } + + &:focus-visible { + outline: 2px solid var(--color-typo-brand); + outline-offset: 2px; + }src/helpers/utils.js (1)
1-7: Consider rounding the TB value and handling the 1000 GB boundary.Two minor display nits:
- Exactly 1000 GB returns
"1000 GB"instead of"1 TB"because the check is>rather than>=.- Values like 1234 GB produce
"1.234 TB"— atoFixed(1)ortoFixed(2)would give cleaner output.Suggested tweak
const gbToTb = (value) => { - if (value > 1000) { - return value / 1000 + ' TB'; + if (value >= 1000) { + return (value / 1000).toFixed(2) + ' TB'; } else { return value + ' GB'; } };src/App.scss (1)
22-26: Hardcoded error color breaks theme consistency.The rest of the file uses CSS variables (e.g.,
var(--color-typo-primary)), but&_errorhardcodes#c62828. This won't adapt to dark/light theme changes.Consider introducing a CSS variable like
--color-errorand referencing it here, consistent with how the sibling&_no-resultsclass usesvar(--color-typo-primary).src/helpers/getData.js (1)
15-42:for…inloops inparseProtocolDatalack own-property guards.
parseNodeData(Line 47) correctly usesObject.hasOwn, but the twofor…inloops here (Lines 18–19) iterate without an own-property check. This is safe for plain JSON objects from the API but inconsistent with the pattern used elsewhere in the same file.♻️ Suggested consistency fix
- for (const protocol in protocols) { - for (const network in protocols[protocol]) { + for (const protocol of Object.keys(protocols)) { + for (const network of Object.keys(protocols[protocol])) {src/helpers/protocolDisplay.js (2)
9-15: Duplicated helpers inscripts/check-protocol-icons.js.
capitalizeFirstLetterandformatProtocolDisplayNameare duplicated nearly verbatim inscripts/check-protocol-icons.js(Lines 28–42 of that file). If the script can import or require from this module (or a shared CJS wrapper), consider consolidating. If the environments are incompatible (ESM vs CJS), the duplication is acceptable but worth a// NOTE:comment linking the two copies.Also applies to: 36-42
58-73: Search aliases may contain duplicates.For single-word display names (e.g.,
"Unichain"), the spaceless variant is identical to the display name, producing duplicate entries. This doesn't affect correctness (.some()still works), but you could add anew Set(...)or[...new Set(arr)]to deduplicate if the alias list grows.src/index.js (1)
7-13:React.createElementworkaround for.jsextension under Vite.This works correctly but is an unusual pattern. Consider renaming the file to
index.jsxso you can use the more readable JSX syntax, updating the<script>tag inindex.htmlaccordingly. This would be consistent with the other.jsxfiles in the project (e.g.,App.jsx,TableWrapper.jsx).src/components/ProtocolIcon/ProtocolIcon.test.jsx (1)
6-26: Consider splitting into individual test cases per protocol.Bundling four assertions into one
itblock means a failure onMonadmasks whetherPlasma/Tempo/Unichainalso pass. Usingit.eachor separateitblocks would give clearer diagnostics on failure.♻️ Example with it.each
- it('renders provided SVG icons for newly added protocols', () => { - const { container: monadContainer } = render( - <ProtocolIcon protocolName="Monad" /> - ); - expect(monadContainer.querySelector('svg')).toBeTruthy(); - - const { container: plasmaContainer } = render( - <ProtocolIcon protocolName="Plasma" /> - ); - expect(plasmaContainer.querySelector('svg')).toBeTruthy(); - - const { container: tempoContainer } = render( - <ProtocolIcon protocolName="Tempo" /> - ); - expect(tempoContainer.querySelector('svg')).toBeTruthy(); - - const { container: unichainContainer } = render( - <ProtocolIcon protocolName="Unichain" /> - ); - expect(unichainContainer.querySelector('svg')).toBeTruthy(); - }); + it.each(['Monad', 'Plasma', 'Tempo', 'Unichain'])( + 'renders an SVG icon for %s', + (protocolName) => { + const { container } = render(<ProtocolIcon protocolName={protocolName} />); + expect(container.querySelector('svg')).toBeTruthy(); + } + );.github/workflows/ci.yml (1)
19-21: Consider pinning Node.js to an LTS minor for reproducibility.
node-version: 20will float to the latest Node 20.x. For a more reproducible CI, consider using a specific LTS version (e.g.,20.xis equivalent here, butlts/*or a pinned version like20.18avoids surprises from patch-level changes).src/App.jsx (1)
4-4: Remove commented-out import.Dead code: the
PlannedUpgradesimport is commented out. Consider removing it to keep the file clean.scripts/check-protocol-icons.js (2)
29-43: Duplicated display-name logic fromprotocolDisplay.js.
capitalizeFirstLetterandformatProtocolDisplayNameare duplicated here. If the canonical versions inprotocolDisplay.jsdiverge, this script will silently produce different display names, causing false positives/negatives in the icon check. Consider extracting shared logic into a CJS-compatible module, or converting this script to ESM (Vite projects typically support it).
45-56: Fragile regex-based icon key extraction.The regex on Line 48 relies on exact two-space indentation and JSX formatting of
ProtocolIcon.jsxto extract icon keys. Any reformatting (e.g., a prettier config change) could silently break the check. Consider exportingiconTypeskeys from a shared data module, or parsing the AST with a lightweight tool.src/components/ProtocolCards/ProtocolCards.jsx (1)
16-48: MoveCardcomponent outside ofProtocolCardsto avoid reconciliation issues.Defining
Cardas a nested function inside the render body creates a new component identity on every render, causing React to unmount/remount rather than update. Extract it to module scope or memoize it.Proposed fix
+const Card = ({ cardHeader, size, client }) => { + return ( + <div className="card"> + <div className="card_header"> + <div>{cardHeader}</div> + </div> + <div className="card_data__block"> + <div className="metadata_wrapper"> + <div className="metadata"> + <span>Size:</span> + <div className="size_value"> + <Tag + icon={<DatabaseOutlined />} + bordered={false} + className="custom_tag" + > + {size ? size : 'No Data'} + </Tag> + </div> + </div> + <div className="metadata"> + <span>Client:</span> + <div> + <Tag color="default" bordered={false}> + {client} + </Tag> + </div> + </div> + </div> + </div> + </div> + ); +}; + export default function ProtocolCards({ data }) { const protocolsArray = Array.from(new Set(data.map((item) => item.protocol))); - - const Card = ({ cardHeader, size, client }) => { - ... - };src/components/TableWrapper/TableWrapper.jsx (1)
153-168: Consider extracting inline styles to CSS/SCSS.The Protocol column render function uses inline
styleobjects (Lines 155, 159–161). These create new object references each render. Since the component already imports SCSS elsewhere, moving these to a stylesheet would be more consistent and avoids unnecessary object allocations.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/check-protocol-icons.js (3)
51-62: Regex-based JSX parsing is fragile;\sin the character class is overly permissive.Two observations on
extractIconKeys:
- The regex assumes exactly 2-space indentation and a specific key–value layout. Any auto-formatter change (e.g., switching to tabs, changing indent depth) will silently break the check.
\sinside the character class['A-Za-z0-9\s.-]also matches\n,\r, and\t, not just spaces. For matching multi-word keys like'BNB Smart Chain', a literal space would be more precise.Low risk in practice, but worth tightening:
Suggested regex tweak
- /^\s{2}(['A-Za-z0-9\s.-]+):\s*</gm + /^\s{2}(['A-Za-z0-9 .-]+):\s*</gm
64-94: No request timeout — CI job can hang indefinitely if the API is unresponsive.If the upstream API stalls without closing the connection, this promise never settles and the CI job hangs until the runner kills it. Adding a timeout is a simple safeguard.
Suggested fix
client - .get(target, (response) => { + .get(target, { timeout: 30_000 }, (response) => { if (response.statusCode < 200 || response.statusCode >= 300) {.on('error', (error) => reject(error)); + .on('timeout', function () { this.destroy(new Error('Request timed out')); });Note: the
timeoutoption sets a socket-idle timeout. You may also wantsetTimeouton the request object for a hard deadline.
35-49: Duplicated helpers fromprotocolDisplay.js— acceptable given CJS/ESM boundary, but worth a note.
capitalizeFirstLetterandformatProtocolDisplayNameare near-copies of their counterparts insrc/helpers/protocolDisplay.js. Since this is a CJS Node script and the app uses ESM, direct import isn't straightforward. Consider adding a brief comment noting the duplication so future maintainers keep them in sync.Also note the subtle difference:
protocolDisplay.jsreturns''for a falsyprotocolSlug, while this version doesn't guard against it. Currently safe becauseObject.entriesalways yields string keys, but worth aligning for defensiveness.
- add CI test step and pin Node version in workflow - switch social meta images to absolute chainstats.org URLs - move testing-library packages to devDependencies - harden icon check script logging and reduce format fragility - remove stale commented import and enforce case-insensitive search matching - guard ProtocolIcon against null/undefined protocol names - rework table sort/filter state to avoid row-span lag during reordering - move protocol cell inline styles into TableWrapper.scss - add focus-visible outline for theme toggle accessibility - make size formatter handle 1000 GB boundary and consistent TB precision - replace hardcoded error color with theme variable - iterate parseProtocolData with Object.keys for consistency - deduplicate protocol search aliases - convert entrypoint to index.jsx and update index.html module path - split ProtocolIcon SVG checks into parameterized tests - move ProtocolCards Card component to module scope
easeev
force-pushed
the
codex/vite-react19-ui-polish
branch
from
d2e7485 to
bf215c6
Compare
- bump GitHub Actions Node.js to 24.13.1 - update README prerequisite from Node.js 20+ to 24+
1 participant
Summary
This PR modernizes the app stack and applies the requested UI/data polish across protocol naming, icon coverage, sorting behavior, and data filtering.
Changes
.github/workflows/ci.yml) and protocol icon coverage check script.BNB Smart ChainPolygon PoStestnettoken from composed display names where appropriate.No datarows in the table.Validation
npm run testpassed (7 tests).npm run buildpassed.npm run check:protocol-iconspassed (27visible protocols).Summary by CodeRabbit
New Features
Bug Fixes / Tests
Documentation
Chores