feat: benchmark details refinement#204
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 39 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMigrates benchmarks and best-practice content to acceptance-test–scoped types and data, adds helpers to generalize and fetch acceptance-test benchmarks, updates scoring/sorting, rewires UI/components/pages to use generalized acceptance-test results, and updates tests/docs accordingly. ChangesAcceptance-test migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refactors the benchmark data model and UI to support per-acceptance-test benchmark results (instead of a single benchmark result per best practice), and updates best-practice technical details to be authored/rendered as JSX/TSX rather than string markdown.
Changes:
- Migrates benchmarks from
AppBenchmarktoAcceptanceTestBenchmarks(record keyed by acceptance-test slug) and updates score/aggregation utilities accordingly. - Introduces
data/acceptance-tests/*(types, registry, utils, tests) and updates UI components/pages to display multiple acceptance-test results per best practice. - Replaces markdown-based technical-details rendering with a React
BestPracticeTechnicalDetailscomponent, and migrates best-practice content files from.tsto.tsx.
Reviewed changes
Copilot reviewed 51 out of 54 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ensawards.org/src/utils/markdown.ts | Removes the custom markdown-to-HTML helper (no longer needed after moving technical details to JSX). |
| ensawards.org/src/pages/leaderboards/[appType].astro | Cleans up unused imports in the leaderboards page. |
| ensawards.org/src/pages/app/[appSlug]/index.astro | Updates app page benchmark display to show acceptance-test-level results grouped under each best practice. |
| ensawards.org/src/pages/app/[appSlug]/[categorySlug]/[bestPracticeSlug]/index.astro | Updates best-practice detail page to handle multiple acceptance-test hero badges and revised benchmark summary/contributions logic. |
| ensawards.org/src/components/organisms/ProtocolBestPracticeDetails.astro | Switches protocol best-practice technical details rendering to the new TSX component. |
| ensawards.org/src/components/organisms/AppBestPracticeDetails.astro | Updates app best-practice details to the acceptance-test benchmark model and new technical details component. |
| ensawards.org/src/components/molecules/BestPracticeTechnicalDetails.tsx | Adds a React component for rendering best-practice technical details authored as JSX. |
| ensawards.org/src/components/molecules/BestPracticeTechnicalDetails.astro | Removes the old markdown-based Astro technical details component. |
| ensawards.org/src/components/atoms/cards/BenchmarkSummaryCard.astro | Updates summary card to display acceptance-test benchmark results and multiple “last updated” timestamps. |
| ensawards.org/src/components/atoms/cards/BenchmarksPerAppTypeCard.tsx | Updates per-app-type benchmark card to render multiple acceptance-test badges per app. |
| ensawards.org/src/components/atoms/cards/AppSummaryCard.tsx | Updates app summary UI to show acceptance-test benchmark items per best practice (nested). |
| ensawards.org/src/components/atoms/cards/AcceptanceTestBenchmarkResultCard.astro | Adds a dedicated card component for acceptance-test benchmark results. |
| ensawards.org/src/components/atoms/badges/BenchmarkResultHeroBadge.astro | Updates hero badge rendering to accept acceptance-test benchmark inputs. |
| ensawards.org/src/components/atoms/badges/BenchmarkResultBadge.tsx | Updates badge typings/helpers to accept acceptance-test benchmarks. |
| ensawards.org/data/shared/test-utils.tsx | Updates mocks to reflect JSX technical details and acceptance-test benchmark objects (with notes). |
| ensawards.org/data/ens-best-practices/utils.ts | Updates scoring/pass-count logic for acceptance-test-level benchmarks; adds best-practice slug sorting helper(s). |
| ensawards.org/data/ens-best-practices/utils.test.ts | Updates best-practice utility tests to the new benchmark shape. |
| ensawards.org/data/ens-best-practices/types.ts | Changes technical details to JSX and makes best-practice benchmarks map to acceptance-test benchmarks. |
| ensawards.org/data/ens-best-practices/index.ts | Expands glob import to include .tsx best-practice definitions. |
| ensawards.org/data/ens-best-practices/display-profiles/mock-bp-all-pending-2.tsx | Adds TSX mock best practice using new technical-details + acceptance-tests structure. |
| ensawards.org/data/ens-best-practices/display-profiles/index.ts | Adjusts Display Profiles category status (currently set to Active for dev). |
| ensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts.tsx | Migrates this best practice to TSX + new technical-details structure. |
| ensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts.ts | Removes old markdown-string version of the best practice. |
| ensawards.org/data/ens-best-practices/contract-naming/mock-bp-all-pending.tsx | Adds TSX mock best practice for pending acceptance-test benchmarks. |
| ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet.tsx | Migrates this best practice to TSX + adds acceptance tests. |
| ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet.ts | Removes old markdown-string version of the best practice. |
| ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains.tsx | Migrates this best practice to TSX + adds acceptance tests. |
| ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains.ts | Removes old markdown-string version of the best practice. |
| ensawards.org/data/contributors/utils.ts | Updates contribution aggregation to count acceptance-test benchmarks. |
| ensawards.org/data/benchmarks/utils.ts | Refactors benchmark utilities for acceptance-test-level scoring, grouping, sorting, timestamps. |
| ensawards.org/data/benchmarks/utils.test.ts | Updates benchmark utility tests for the new benchmark shape. |
| ensawards.org/data/benchmarks/types.ts | Replaces AppBenchmark with AcceptanceTestBenchmarks (record of acceptance-test benchmarks). |
| ensawards.org/data/benchmarks/index.ts | Updates registry glob to load benchmarks.tsx files. |
| ensawards.org/data/benchmarks/index.test.ts | Strengthens registry tests for applicability and acceptance-test key coverage and notes presence. |
| ensawards.org/data/apps/walletchan-wallet/benchmarks.tsx | Migrates app benchmark data to acceptance-test-level entries (with notes). |
| ensawards.org/data/apps/walletchan-wallet/benchmarks.ts | Removes old best-practice-level benchmark data file. |
| ensawards.org/data/apps/utils.ts | Updates app score calculation to average across completed acceptance-test benchmarks. |
| ensawards.org/data/apps/utils.test.ts | Updates app utils tests to the new benchmark model. |
| ensawards.org/data/apps/rainbow-wallet/benchmarks.tsx | Migrates app benchmark data to acceptance-test-level entries (with notes). |
| ensawards.org/data/apps/rainbow-wallet/benchmarks.ts | Removes old best-practice-level benchmark data file. |
| ensawards.org/data/apps/metamask-wallet/benchmarks.tsx | Updates benchmark data to the acceptance-test-level shape (pending entries). |
| ensawards.org/data/apps/etherscan-explorer/benchmarks.tsx | Migrates app benchmark data to acceptance-test-level entries (with notes). |
| ensawards.org/data/apps/etherscan-explorer/benchmarks.ts | Removes old best-practice-level benchmark data file. |
| ensawards.org/data/apps/coinbase-wallet/benchmarks.tsx | Migrates app benchmark data to acceptance-test-level entries (with notes). |
| ensawards.org/data/apps/coinbase-wallet/benchmarks.ts | Removes old best-practice-level benchmark data file. |
| ensawards.org/data/apps/blockscout-explorer/benchmarks.tsx | Migrates app benchmark data to acceptance-test-level entries (with notes). |
| ensawards.org/data/apps/blockscout-explorer/benchmarks.ts | Removes old best-practice-level benchmark data file. |
| ensawards.org/data/acceptance-tests/utils.ts | Adds acceptance-test lookup + per-app acceptance-test benchmark aggregation utilities. |
| ensawards.org/data/acceptance-tests/types.ts | Introduces acceptance-test and acceptance-test benchmark types (including notes). |
| ensawards.org/data/acceptance-tests/index.ts | Adds a registry of acceptance tests derived from best practices. |
| ensawards.org/data/acceptance-tests/index.test.ts | Adds tests for acceptance-test slug validity and uniqueness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ensawards.org/src/components/organisms/ProtocolBestPracticeDetails.astro (1)
128-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded
.tsextension in the GitHubgitHubTargetHreflink will 404 for.tsxbest-practice files.This PR converts most protocol best-practice definition files from
.tsto.tsx. However, the link at Line 128 uses a hardcoded.tsextension, causing 404s for any.tsxbest practice (e.g.,name-your-smart-contracts.tsx).The codebase currently has a mix: 5 files are
.tsxbut at least one (recognize-all-ens-names.ts) remains.ts. Simply changing the extension to.tsxwould break the.tsfile. The extension needs to be made dynamic—either store it in the best practice metadata or determine it programmatically.🤖 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 `@ensawards.org/src/components/organisms/ProtocolBestPracticeDetails.astro` around lines 128 - 129, The GitHub link currently hardcodes ".ts" causing 404s for ".tsx" files; update the gitHubTargetHref template to use a dynamic extension property (e.g., bestPractice.fileExtension) and fall back to ".ts" when missing: construct the URL using bestPractice.category.categorySlug + "/" + bestPractice.bestPracticeSlug + (bestPractice.fileExtension ?? ".ts"); ensure the source of metadata (where bestPractice objects are created) is updated to include fileExtension for .tsx entries or the fallback will handle remaining .ts files so links resolve correctly.
🤖 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.
Inline comments:
In `@ensawards.org/data/acceptance-tests/index.test.ts`:
- Around line 1-25: The test currently validates slugs derived from
ENS_BEST_PRACTICES instead of asserting the exported flattened ACCEPTANCE_TESTS;
import ACCEPTANCE_TESTS from "data/acceptance-tests/index" and (1) build the
flattened list from ENS_BEST_PRACTICES exactly as the module does, (2) assert
the flattened list equals the exported ACCEPTANCE_TESTS (same length and same
items/order or same set), and then run isValidSlug and areStringsUnique checks
against ACCEPTANCE_TESTS (use a single slugArray for the whole suite, not per
bestPractice) so changes to the exported constant will fail the test.
In `@ensawards.org/data/apps/blockscout-explorer/benchmarks.tsx`:
- Around line 63-65: The pending fixture keys are incorrect: replace the
incorrect mock benchmark slug "mock-bp-all-pending-2" and the acceptance-test
key "mock-acceptance-test-3" with the registered slugs used elsewhere
("mock-bp-all-pending" and "mock-acceptance-test-1") so the pending fixture
matches the real registered graph; update the object keys in the benchmarks
fixture where "mock-bp-all-pending-2" and "mock-acceptance-test-3" appear to
"mock-bp-all-pending" and "mock-acceptance-test-1" respectively.
In `@ensawards.org/data/apps/etherscan-explorer/benchmarks.tsx`:
- Around line 41-43: The benchmark entry uses incorrect mock slugs; update the
keys in benchmarks.tsx so the best-practice slug "mock-bp-all-pending-2" is
changed to the registered "mock-bp-all-pending" and the acceptance test key
"mock-acceptance-test-3" is changed to the registered "mock-acceptance-test-1"
so the entry will join to the defined acceptance test (refer to the object that
contains "mock-bp-all-pending-2" / "mock-acceptance-test-3" and replace those
strings).
In `@ensawards.org/data/apps/walletchan-wallet/benchmarks.tsx`:
- Line 5: Replace the placeholder benchmark results and borrowed proof asset
with pending/undefined entries and remove the fake image import: delete or
comment out the import exampleProofImage from
"data/apps/blockscout-explorer/acceptance-test-benchmark-proof-example.png" and
update the benchmark data objects (the entries currently marked "Pass" in the
benchmarks array/const between lines 15-58) to use a pending/undefined status
and clear placeholder notes/proof fields; ensure any references to
exampleProofImage in the component or export are removed or replaced with null
so the UI shows no verified proof until real WalletChan evidence is available.
In `@ensawards.org/data/benchmarks/utils.ts`:
- Around line 28-49: The function getAppBenchmarksByBestPractice indexes
APP_BENCHMARKS[appSlug][slug] and asserts an AcceptanceTestBenchmarks return
without checking the lookup; change it to validate that appBenchmarks[slug] is
defined before pushing (if undefined throw an explicit invariant Error
mentioning slug and appSlug) so callers never receive a missing value; apply the
same defensive check to the other helper in this file that also indexes into
APP_BENCHMARKS (the sibling function that performs appBenchmarks[...] lookups)
and ensure both functions either throw on missing entries or widen their return
types consistently—prefer throwing here and include clear context in the error
message.
In
`@ensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts.tsx`:
- Around line 56-64: The JSX text node ending with "requires the contract to
implement" is immediately followed by the Ownable <a> element, causing the
newline to be stripped and rendering "implementOwnable"; fix it by inserting an
explicit space token {" "} between the text node and the <a> element so the
rendered output reads "implement Ownable" (update the JSX near the Ownable link
element).
In `@ensawards.org/data/ens-best-practices/display-profiles/index.ts`:
- Line 14: Change the temporary development value of the status field back to
the intended release state: replace the current assignment of status:
CategoryStatuses.Active with status: CategoryStatuses.ComingSoon (or the
project's canonical draft enum value) in the module where the display profile is
defined so the exported status variable (status / CategoryStatuses) reflects the
draft state before merging.
In `@ensawards.org/data/ens-best-practices/utils.ts`:
- Around line 104-126: The current calcBestPracticeScore flattens all app
benchmarks (via getAppBenchmarksByBestPractice(...).flatMap(...)) and averages
acceptance-test points, which weights apps by number of completed tests; change
it to compute each app's average score first and then average those per-app
scores so each app is equally weighted: iterate the array returned by
getAppBenchmarksByBestPractice(bestPractice.bestPracticeSlug), for each
appBenchmark compute that app's sum of calcEnsAwardsPoints(...) and count of
defined acceptance tests to get the appScore (skip apps with zero defined
tests), collect appScores, then compute the final score as
Math.round(sum(appScores) / appScores.length) (handle zero-apps by returning
undefined). Update calcBestPracticeScore to use this per-app aggregation and
return the rounded score as before.
In
`@ensawards.org/src/components/atoms/cards/AcceptanceTestBenchmarkResultCard.astro`:
- Around line 40-42: The desktop heading in
AcceptanceTestBenchmarkResultCard.astro uses a non-standard Tailwind class
"text-md" which is ignored; update the H3 that renders {acceptanceTest.name} to
use "text-base" (to match Tailwind's scale and be consistent with the mobile
heading that uses "text-lg") so the font-size is applied correctly.
In `@ensawards.org/src/components/atoms/cards/AppSummaryCard.tsx`:
- Around line 116-147: The mapped anchor inside AppSummaryCard (the <a> returned
for each acceptanceTest in the Object.entries(appBenchmarks).map) is missing a
React key; add a stable unique key prop to that element (e.g. compose one from
acceptanceTest.id or acceptanceTest.name plus bestPractice.bestPracticeSlug or
app.appSlug) so React can reconcile correctly—locate the map where appBenchmarks
is iterated and set key on the <a> using those identifiers.
In `@ensawards.org/src/components/organisms/AppBestPracticeDetails.astro`:
- Around line 26-35: The code assumes
getAppBenchmarks(app.appSlug)[bestPractice.bestPracticeSlug] always exists; add
an explicit guard when building appsWithBenchmarks: compute const
matchingBenchmark =
getAppBenchmarks(app.appSlug)[bestPractice.bestPracticeSlug], then if
(!matchingBenchmark) either throw a clear Error referencing app.appSlug and
bestPractice.bestPracticeSlug or skip this app (e.g., continue/filter) so
downstream users like sorter and benchmarkContributions (which call
Object.keys/values) never receive undefined; apply the same invariant/guard at
the other occurrences noted (around lines where acceptanceTestBenchmarks is
assigned).
---
Outside diff comments:
In `@ensawards.org/src/components/organisms/ProtocolBestPracticeDetails.astro`:
- Around line 128-129: The GitHub link currently hardcodes ".ts" causing 404s
for ".tsx" files; update the gitHubTargetHref template to use a dynamic
extension property (e.g., bestPractice.fileExtension) and fall back to ".ts"
when missing: construct the URL using bestPractice.category.categorySlug + "/" +
bestPractice.bestPracticeSlug + (bestPractice.fileExtension ?? ".ts"); ensure
the source of metadata (where bestPractice objects are created) is updated to
include fileExtension for .tsx entries or the fallback will handle remaining .ts
files so links resolve correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 094c942e-4dcc-43bd-8f97-62be63cbeb64
⛔ Files ignored due to path filters (3)
ensawards.org/data/apps/blockscout-explorer/acceptance-test-benchmark-proof-example.pngis excluded by!**/*.pngensawards.org/data/ens-best-practices/contract-naming/content-images/enscribe-lookup-example.pngis excluded by!**/*.pngensawards.org/data/ens-best-practices/contract-naming/content-images/mainnet-interactions-display-named-smart-contracts-example.pngis excluded by!**/*.png
📒 Files selected for processing (51)
ensawards.org/data/acceptance-tests/index.test.tsensawards.org/data/acceptance-tests/index.tsensawards.org/data/acceptance-tests/types.tsensawards.org/data/acceptance-tests/utils.tsensawards.org/data/apps/blockscout-explorer/benchmarks.tsensawards.org/data/apps/blockscout-explorer/benchmarks.tsxensawards.org/data/apps/coinbase-wallet/benchmarks.tsensawards.org/data/apps/coinbase-wallet/benchmarks.tsxensawards.org/data/apps/etherscan-explorer/benchmarks.tsensawards.org/data/apps/etherscan-explorer/benchmarks.tsxensawards.org/data/apps/metamask-wallet/benchmarks.tsxensawards.org/data/apps/rainbow-wallet/benchmarks.tsensawards.org/data/apps/rainbow-wallet/benchmarks.tsxensawards.org/data/apps/utils.test.tsensawards.org/data/apps/utils.tsensawards.org/data/apps/walletchan-wallet/benchmarks.tsensawards.org/data/apps/walletchan-wallet/benchmarks.tsxensawards.org/data/benchmarks/index.test.tsensawards.org/data/benchmarks/index.tsensawards.org/data/benchmarks/types.tsensawards.org/data/benchmarks/utils.test.tsensawards.org/data/benchmarks/utils.tsensawards.org/data/contributors/utils.tsensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains.tsensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains.tsxensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet.tsensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet.tsxensawards.org/data/ens-best-practices/contract-naming/mock-bp-all-pending.tsxensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts.tsensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts.tsxensawards.org/data/ens-best-practices/display-profiles/index.tsensawards.org/data/ens-best-practices/display-profiles/mock-bp-all-pending-2.tsxensawards.org/data/ens-best-practices/index.tsensawards.org/data/ens-best-practices/types.tsensawards.org/data/ens-best-practices/utils.test.tsensawards.org/data/ens-best-practices/utils.tsensawards.org/data/shared/test-utils.tsxensawards.org/src/components/atoms/badges/BenchmarkResultBadge.tsxensawards.org/src/components/atoms/badges/BenchmarkResultHeroBadge.astroensawards.org/src/components/atoms/cards/AcceptanceTestBenchmarkResultCard.astroensawards.org/src/components/atoms/cards/AppSummaryCard.tsxensawards.org/src/components/atoms/cards/BenchmarkSummaryCard.astroensawards.org/src/components/atoms/cards/BenchmarksPerAppTypeCard.tsxensawards.org/src/components/molecules/BestPracticeTechnicalDetails.astroensawards.org/src/components/molecules/BestPracticeTechnicalDetails.tsxensawards.org/src/components/organisms/AppBestPracticeDetails.astroensawards.org/src/components/organisms/ProtocolBestPracticeDetails.astroensawards.org/src/pages/app/[appSlug]/[categorySlug]/[bestPracticeSlug]/index.astroensawards.org/src/pages/app/[appSlug]/index.astroensawards.org/src/pages/leaderboards/[appType].astroensawards.org/src/utils/markdown.ts
💤 Files with no reviewable changes (11)
- ensawards.org/src/utils/markdown.ts
- ensawards.org/data/apps/rainbow-wallet/benchmarks.ts
- ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet.ts
- ensawards.org/data/apps/walletchan-wallet/benchmarks.ts
- ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains.ts
- ensawards.org/data/apps/blockscout-explorer/benchmarks.ts
- ensawards.org/src/components/molecules/BestPracticeTechnicalDetails.astro
- ensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts.ts
- ensawards.org/data/apps/coinbase-wallet/benchmarks.ts
- ensawards.org/src/pages/leaderboards/[appType].astro
- ensawards.org/data/apps/etherscan-explorer/benchmarks.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ensawards.org/data/benchmarks/utils.ts (1)
35-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSilent empty return when a non-App BestPractice slug is passed.
The filter at line 51 only includes apps when
bestPractice.type === BestPracticeTypes.App. If aProtocolbest-practice slug is supplied, the function silently returns[]rather than throwing — opposite to the invariant-throwing pattern used everywhere else in this file. Either narrow the parameter type toBestPracticeApp["bestPracticeSlug"], or throw an invariant whenbestPractice.type !== BestPracticeTypes.Appso misuse fails loudly.🛡️ Proposed invariant
const bestPractice = getBestPracticeBySlug(slug); if (bestPractice === undefined) { throw new Error(`Invariant(BestPracticeSlug): BestPractice with slug ${slug} is not defined`); } + if (bestPractice.type !== BestPracticeTypes.App) { + throw new Error( + `Invariant(BestPracticeType): getAppBenchmarksByBestPractice expects an App best practice, got ${bestPractice.type} for slug ${slug}`, + ); + } + const benchmarks: AcceptanceTestBenchmarks[] = []; for (const [appSlug, appBenchmarks] of Object.entries(APP_BENCHMARKS)) { const app = getAppBySlug(appSlug); if (app === undefined) { throw new Error(`Invariant(AppSlug): App with slug ${appSlug} is not defined`); } - if (bestPractice.type === BestPracticeTypes.App && bestPractice.appliesTo.includes(app.type)) { + if (bestPractice.appliesTo.includes(app.type)) {🤖 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 `@ensawards.org/data/benchmarks/utils.ts` around lines 35 - 65, getAppBenchmarksByBestPractice currently accepts any BestPracticeSlug but only handles BestPracticeTypes.App and silently returns [] for other types; update the function to fail loudly by checking the resolved bestPractice (from getBestPracticeBySlug) and throwing an invariant if bestPractice.type !== BestPracticeTypes.App (use the same Error style as existing invariants, e.g., "Invariant(BestPracticeType): ...") so misuse is detected, or alternatively restrict the input type to the App-specific slug type (BestPracticeApp["bestPracticeSlug"]) — choose one approach and apply it consistently in getAppBenchmarksByBestPractice and any callers.
🤖 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.
Inline comments:
In `@ensawards.org/data/benchmarks/index.test.ts`:
- Around line 97-126: The test currently filters out entries with undefined
benchmark values using the inline type guard in
Object.entries(appBenchmark).filter(...), allowing acceptanceTestSlug: undefined
to bypass assertions; remove that filter (or replace it with an explicit
assertion) so every entry in appBenchmark is validated: iterate over
Object.entries(appBenchmark) directly in the loop that checks benchmark.result,
benchmark.contributions, and benchmark.notes, or add a clear
expect(benchmark).toBeDefined() before other checks to fail loudly when an entry
is undefined; reference the Object.entries(appBenchmarks) outer loop and the
inner handling of ([acceptanceTestSlug, benchmark]) when making the change.
- Around line 87-90: The test currently uses an order-sensitive equality check
on acceptanceTestSlugs vs acceptanceTestSlugsByBestPractice which makes the test
brittle; update the assertion in the test that references acceptanceTestSlugs
and acceptanceTestSlugsByBestPractice (derived from appBenchmark and
bestPractice.technicalDetails.acceptanceTests) to perform an order-insensitive
comparison instead — e.g., convert both arrays to sets or sort both arrays
before comparing so only membership/coverage is asserted (not ordering), and
keep the assertion message aligned with the change.
In
`@ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet.tsx`:
- Around line 166-180: The acceptance test entry with acceptanceTestSlug
"mainnet-interactions-display-named-smart-contracts-at2" and name "Acceptance
Test 2" is a placeholder that duplicates the first AT body; replace it with a
unique, descriptive test (update the name from "Acceptance Test 2" to a
meaningful title), replace the placeholder description JSX in the description
field with the full test steps/expectations (and add or reference the
appropriate illustrative image instead of the "TEST DESCRIPTION WITHOUT IMAGE"
TODO), and ensure the slug remains unique and aligned with the new name so this
placeholder content cannot be shipped when the PR leaves Draft.
In
`@ensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts.tsx`:
- Around line 130-133: The acceptance-test image tag rendering
enscribelookupAcceptanceTestImage.src lacks intrinsic sizing and can cause CLS;
update the <img> in name-your-smart-contracts.tsx to use the same sizing as its
sibling (e.g., add className="w-auto h-[800px]" or explicit width and height
attributes) so the enscribelookupAcceptanceTestImage reserves layout space
consistently with display-named-smart-contracts-mainnet.tsx.
In `@ensawards.org/data/ens-best-practices/utils.ts`:
- Around line 133-148: isAppPassing currently returns true for an empty
AcceptanceTestBenchmarks because the loop never runs; update isAppPassing to
first ensure there is at least one benchmarked acceptance test (e.g., count
Object.values(appBenchmarks) for any non-undefined entries) and return false if
none exist, then proceed to verify each acceptanceTestBenchmark.result equals
BenchmarkResults.Pass or BenchmarkResults.PartialPass; reference isAppPassing
and AcceptanceTestBenchmarks so callers like calcAppsPassed will no longer treat
empty records as passing.
In
`@ensawards.org/src/components/atoms/cards/AcceptanceTestBenchmarkResultCard.astro`:
- Line 15: Replace the inline TODO in the AcceptanceTestBenchmarkResultCard
component with a concrete next step: either (A) create a tracking issue titled
"Refine AcceptanceTestBenchmarkResultCard design" and update the TODO to
reference that issue number and list basic acceptance criteria, or (B) scaffold
a minimal, accessible layout in AcceptanceTestBenchmarkResultCard that uses
semantic elements (article/header/section), props currently used by the
component, and placeholder classNames for future design (so the component
renders a stable structure rather than a comment). Update the TODO line to
reference the issue if you create one, or remove it after adding the scaffold.
- Around line 32-34: The commented-out <p> blocks in
AcceptanceTestBenchmarkResultCard.astro reference the no-longer-in-scope
bestPractice.description; update both commented occurrences to use
acceptanceTest.description (the field on AcceptanceTest) so the comment reflects
valid props — search for bestPractice.description inside the component and
replace with acceptanceTest.description, ensuring AcceptanceTest/acceptanceTest
is the referenced symbol.
---
Outside diff comments:
In `@ensawards.org/data/benchmarks/utils.ts`:
- Around line 35-65: getAppBenchmarksByBestPractice currently accepts any
BestPracticeSlug but only handles BestPracticeTypes.App and silently returns []
for other types; update the function to fail loudly by checking the resolved
bestPractice (from getBestPracticeBySlug) and throwing an invariant if
bestPractice.type !== BestPracticeTypes.App (use the same Error style as
existing invariants, e.g., "Invariant(BestPracticeType): ...") so misuse is
detected, or alternatively restrict the input type to the App-specific slug type
(BestPracticeApp["bestPracticeSlug"]) — choose one approach and apply it
consistently in getAppBenchmarksByBestPractice and any callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15b38a2c-4fe8-47de-ae8b-0e003a8d3365
📒 Files selected for processing (11)
ensawards.org/data/acceptance-tests/index.test.tsensawards.org/data/benchmarks/index.test.tsensawards.org/data/benchmarks/utils.tsensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet.tsxensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts.tsxensawards.org/data/ens-best-practices/utils.tsensawards.org/src/components/atoms/badges/BenchmarkResultBadge.tsxensawards.org/src/components/atoms/badges/BenchmarkResultHeroBadge.astroensawards.org/src/components/atoms/cards/AcceptanceTestBenchmarkResultCard.astroensawards.org/src/components/atoms/cards/AppSummaryCard.tsxensawards.org/src/components/organisms/AppBestPracticeDetails.astro
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 69 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (3)
ensawards.org/data/apps/utils.ts:5
getAppBenchmarksis imported but no longer used after the refactor togetAcceptanceTestBenchmarksByApp. Remove the unused import to avoidnoUnusedLocals/lint failures.
import { getAcceptanceTestBenchmarksByApp } from "data/acceptance-tests/utils.ts";
import { AWARDS } from "data/awards/index.ts";
import type { Award } from "data/awards/types.ts";
import { calcEnsAwardsPoints, getAppBenchmarks } from "data/benchmarks/utils.ts";
import { EntityMetadataTypes } from "data/entity-metadata/types.ts";
ensawards.org/data/shared/test-utils.tsx:3
AcceptanceTestSlugandAcceptanceTestBenchmarksare imported but not used in this test helper. Please remove unused type imports to keep the file passing strict TypeScript unused-import checks.
ensawards.org/data/apps/metamask-wallet/benchmarks/index.tsx:11BenchmarkResults,contributors, andparseTimestampare currently unused because all benchmark entries in this file areundefined(and the only usage is in commented-out code). With strict TS/lint this can fail the build; either remove these imports or add a minimal non-commented usage.
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ensawards.org/src/pages/app/[appSlug]/index.astro (1)
216-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale
gitHubTargetHref— points to removedbenchmarks.tspathThis PR moves per-app benchmark files from
benchmarks.tstobenchmarks/index.tsx, but this URL still references the old filename. The "Contribute" link will 404 on GitHub for every app after this PR merges.🔗 Proposed fix
- gitHubTargetHref={`https://github.com/namehash/ensawards/blob/main/ensawards.org/data/apps/${app.appSlug}/benchmarks.ts`} + gitHubTargetHref={`https://github.com/namehash/ensawards/blob/main/ensawards.org/data/apps/${app.appSlug}/benchmarks/index.tsx`}🤖 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 `@ensawards.org/src/pages/app/`[appSlug]/index.astro around lines 216 - 217, Update the stale gitHubTargetHref prop value which currently points to benchmarks.ts so the "Contribute" link doesn't 404; change the template string used in the gitHubTargetHref prop (the JSX attribute in the component rendering the link) to reference benchmarks/index.tsx (e.g. update `${app.appSlug}/benchmarks.ts` to `${app.appSlug}/benchmarks/index.tsx`) so each app's GitHub link points to the new file location.
🤖 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.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 320: Update the sentence referencing AcceptanceTestBenchmark to point to
the correct types file and use a declarative phrasing: replace the current
"Where `AcceptanceTestBenchmark` represents..." line with something like
"`AcceptanceTestBenchmark` is defined in the benchmarks types file at
ensawards.org/data/benchmarks/types.ts." Locate the reference to
AcceptanceTestBenchmark in CONTRIBUTING.md and change the path from
acceptance-tests/types.ts to benchmarks/types.ts and adjust wording to a simple
declarative statement.
In `@ensawards.org/data/acceptance-tests/utils.ts`:
- Around line 31-63: The JSDoc for generalizeAcceptanceTestBenchmarks is
inaccurate: it links to the wrong symbol (uses
BenchmarkResult.Pass/Fail/PartialPass instead of the actual value object
BenchmarkResults) and describes "all" semantics while the implementation uses
any/some logic. Update the docblock to reference BenchmarkResults (plural) in
the {`@link`} tags and rewrite the bullets to reflect the actual evaluation order
implemented by the function: if any Pass/PartialPass and any Fail → PartialPass;
else if any Pass/PartialPass → Pass; else if any Fail → Fail; else return
undefined (all pending).
In `@ensawards.org/data/apps/metamask-wallet/benchmarks/index.tsx`:
- Line 19: Create a tracking issue that enumerates each placeholder benchmark
entry referenced by the TODO comment "// TODO: remember to rollback to
benchmarks actuall results" across the three wallet benchmark files (metamask,
rainbow, coinbase), list the specific benchmark keys/entries that must be
restored to real results, add the issue number to each TODO comment in the files
for traceability, and reference that issue in the PR description so the rollback
cannot be missed before merge.
In `@ensawards.org/data/benchmarks/utils.ts`:
- Around line 21-30: The function getAppBenchmarks narrows APP_BENCHMARKS[slug]
into the local variable appBenchmarks but then returns APP_BENCHMARKS[slug]
again, losing the narrowed type; change the return to return the
already-narrowed appBenchmarks variable so the undefined guard is respected
(i.e., after the if (appBenchmarks === undefined) throw ...) return
appBenchmarks instead of re-indexing APP_BENCHMARKS[slug]).
In
`@ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains/technicalDetails.tsx`:
- Around line 24-26: The paragraph text in technicalDetails.tsx that reads
"There are several libraries to choose from that support ENSIP-19 and all ENS
best practices" is missing terminal punctuation; update that <p> (the paragraph
node containing that sentence) to end with a period or colon so the sentence is
grammatically complete before the following <ul>, e.g. append "." or ":" to the
sentence text in the component.
In
`@ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet/index.ts`:
- Around line 40-43: The current test object uses a placeholder name "Acceptance
Test 2" for the entry with acceptanceTestSlug
"mainnet-interactions-display-named-smart-contracts-at2"; update the name
property to a descriptive label that explains what the scenario validates (e.g.,
something referencing display of named smart contracts on mainnet or the
specific UI behavior tested) so users see a meaningful title alongside
mainnetInteractionsDisplayNamedSmartContractsAt2Description in the UI.
In
`@ensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet/technicalDetails.tsx`:
- Line 8: The file technicalDetails.tsx currently contains an inline TODO
comment "// TODO: The content isn't fully curated for now." that marks non-final
copy; remove or replace this inline TODO and instead create a tracked issue
(e.g., in your repo tracker) for content curation that references the AT2
placeholder and this best-practice page, then update technicalDetails.tsx to
reference that issue (issue number or short URL) and/or add a brief, stable
placeholder note pointing to the created issue so the PR no longer ships
untracked TODOs; look for the TODO in technicalDetails.tsx and the AT2
placeholder text when making the change.
- Around line 124-131: The exported JSX constant
mainnetInteractionsDisplayNamedSmartContractsAt2Description currently contains
the literal placeholder "TEST DESCRIPTION WITHOUT IMAGE" which will reach
production; replace that literal inside the returned <p> with the real curated
AT2 description (preserving the surrounding <div
className={acceptanceTestDetailsContainerStyles}> wrapper) or, if content isn't
ready, remove or null-export this constant and decouple it from the acceptance
test wiring in display-named-smart-contracts-mainnet/index.ts so the import no
longer provides placeholder content to the acceptance test.
In
`@ensawards.org/data/ens-best-practices/display-profiles/mock-bp-all-pending-2/index.ts`:
- Around line 24-26: The description string for the mock profile contains a
typo: change the value of the description property (the "description" field in
mock-bp-all-pending-2's export) from "Mock best practice with all benchmarks in
pending state. This is used to test the display of pending benchmarks in the UI
with a best pratice other than Contract Naming." to use the correct spelling
"practice" (i.e., replace "pratice" → "practice") so the user-visible text is
correct.
In `@ensawards.org/data/ens-best-practices/utils.ts`:
- Around line 110-122: The variable bestPracticeBenchmarks no longer represents
per-app benchmark records after flatMap(Object.values) — it is a flat array of
AcceptanceTestBenchmark | undefined; rename bestPracticeBenchmarks to
acceptanceTestBenchmarks (and update any uses) to reflect that it contains
per-acceptance-test benchmarks, then adjust the loop variable if needed
(acceptanceTestBenchmark) and keep the existing logic that filters undefined and
calls calcEnsAwardsPoints on each item returned by
getAppBenchmarksByBestPractice.
In `@ensawards.org/src/components/atoms/cards/AppSummaryCard.tsx`:
- Around line 64-75: The constructed generalizedBenchmark unsafely fabricates an
AcceptanceTestBenchmark by grabbing bestPracticeBenchmarks[Object.keys(...)[0]]
which may be undefined; instead either narrow generalizedBenchmark's type to
only { result: BenchmarkResult } | undefined (since only `.result` is read) or
find the first defined entry in bestPracticeBenchmarks to source
contributions/notes; update the code in the block that builds
generalizedBenchmark (and related utilities like sortBenchmarks and the
AcceptanceTestBenchmark import/usage) to stop casting with `as
AcceptanceTestBenchmark` and either (A) return only the result field or (B)
select the first defined benchmark value before reading contributions/notes so
you never access undefined values.
In `@ensawards.org/src/components/molecules/BenchmarkTechnicalDetails.tsx`:
- Around line 34-42: The current mapping over
Object.entries(acceptanceTestBenchmarks) relies on insertion order and can
produce unstable UI ordering; instead iterate deterministically using the
canonical list BestPractice.technicalDetails.acceptanceTests (or the prop that
defines the canonical acceptance test order) and for each entry obtain the
matching benchmark from acceptanceTestBenchmarks by slug, using
getAcceptanceTestBySlug to resolve metadata; preserve the existing
invariant/error handling if a slug from the canonical list has no corresponding
acceptanceTestBenchmark (or decide to skip missing slugs explicitly), and update
the code in BenchmarkTechnicalDetails to map over that canonical array rather
than Object.entries(acceptanceTestBenchmarks).
In
`@ensawards.org/src/pages/app/`[appSlug]/[categorySlug]/[bestPracticeSlug]/index.astro:
- Around line 68-72: The ternary guarding allContributions is redundant because
appAcceptanceTestBenchmarks is initialized as {} and only set via
getAppAcceptanceTestBenchmarks, so simplify by removing the conditional and
directly compute allContributions using
Object.values(appAcceptanceTestBenchmarks).flatMap(...); if
getAppAcceptanceTestBenchmarks can actually return undefined instead, update its
return type/signature accordingly and keep a proper undefined check instead of
the always-true ternary; locate usages in the file referencing allContributions
and the getAppAcceptanceTestBenchmarks call to apply the change.
- Line 59: The file declares allContributions: Contribution[] but never imports
the Contribution type; add a TypeScript type import for Contribution (the
exported interface/type that defines a contribution) at the top of the module
and use it for the allContributions annotation; specifically import the
Contribution type from the module that defines it (where your shared
types/models live) so the symbol Contribution is resolved for the variable
allContributions.
In `@ensawards.org/src/pages/app/`[appSlug]/index.astro:
- Around line 151-166: The current construction of generalizedBenchmark uses
Object.keys(bestPracticeBenchmarks)[0] assuming that first entry is
non-undefined; instead find the first acceptance-test benchmark in
bestPracticeBenchmarks that is defined and use its contributions/notes. Update
the generalizedBenchmark creation (where generalizedBenchmarkResult,
bestPracticeBenchmarks and Object.keys(...) are referenced) to locate the first
key with a truthy value (e.g., using Array.prototype.find on
Object.keys(bestPracticeBenchmarks) to pick the first k where
bestPracticeBenchmarks[k] !== undefined) and pull contributions/notes from that
entry so they aren’t silently undefined when the first key is pending.
---
Outside diff comments:
In `@ensawards.org/src/pages/app/`[appSlug]/index.astro:
- Around line 216-217: Update the stale gitHubTargetHref prop value which
currently points to benchmarks.ts so the "Contribute" link doesn't 404; change
the template string used in the gitHubTargetHref prop (the JSX attribute in the
component rendering the link) to reference benchmarks/index.tsx (e.g. update
`${app.appSlug}/benchmarks.ts` to `${app.appSlug}/benchmarks/index.tsx`) so each
app's GitHub link points to the new file location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5acdf61e-86e6-4173-8685-f125469d78c9
⛔ Files ignored due to path filters (11)
ensawards.org/data/apps/blockscout-explorer/benchmarks/acceptance-test-benchmark-proof-example.pngis excluded by!**/*.pngensawards.org/data/apps/coinbase-wallet/benchmarks/acceptance-test-benchmark-proof-example.pngis excluded by!**/*.pngensawards.org/data/apps/etherscan-explorer/benchmarks/acceptance-test-benchmark-proof-example.pngis excluded by!**/*.pngensawards.org/data/apps/metamask-wallet/benchmarks/acceptance-test-benchmark-proof-example.pngis excluded by!**/*.pngensawards.org/data/apps/rainbow-wallet/benchmarks/acceptance-test-benchmark-proof-example.pngis excluded by!**/*.pngensawards.org/data/apps/walletchan-wallet/benchmarks/acceptance-test-benchmark-proof-example.pngis excluded by!**/*.pngensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains/images/mock-l2-chains-interactions-display-named-smart-contracts-example.pngis excluded by!**/*.pngensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet/images/mainnet-interactions-display-named-smart-contracts-example.pngis excluded by!**/*.pngensawards.org/data/ens-best-practices/contract-naming/mock-bp-all-pending/images/mock-acceptance-test-image.pngis excluded by!**/*.pngensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts/images/enscribe-lookup-example.pngis excluded by!**/*.pngensawards.org/data/ens-best-practices/display-profiles/mock-bp-all-pending-2/images/mock-acceptance-test-image.pngis excluded by!**/*.png
📒 Files selected for processing (37)
CONTRIBUTING.mdensawards.org/data/acceptance-tests/types.tsensawards.org/data/acceptance-tests/utils.tsensawards.org/data/apps/blockscout-explorer/benchmarks/index.tsxensawards.org/data/apps/coinbase-wallet/benchmarks/index.tsxensawards.org/data/apps/etherscan-explorer/benchmarks/index.tsxensawards.org/data/apps/metamask-wallet/benchmarks/index.tsxensawards.org/data/apps/rainbow-wallet/benchmarks/index.tsxensawards.org/data/apps/walletchan-wallet/benchmarks/index.tsxensawards.org/data/benchmarks/index.test.tsensawards.org/data/benchmarks/index.tsensawards.org/data/benchmarks/types.tsensawards.org/data/benchmarks/utils.test.tsensawards.org/data/benchmarks/utils.tsensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains/index.tsensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-l2-chains/technicalDetails.tsxensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet/index.tsensawards.org/data/ens-best-practices/contract-naming/display-named-smart-contracts-mainnet/technicalDetails.tsxensawards.org/data/ens-best-practices/contract-naming/mock-bp-all-pending/index.tsensawards.org/data/ens-best-practices/contract-naming/mock-bp-all-pending/technicalDetails.tsxensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts/index.tsensawards.org/data/ens-best-practices/contract-naming/name-your-smart-contracts/technicalDetails.tsxensawards.org/data/ens-best-practices/display-profiles/mock-bp-all-pending-2/index.tsensawards.org/data/ens-best-practices/display-profiles/mock-bp-all-pending-2/technicalDetails.tsxensawards.org/data/ens-best-practices/index.tsensawards.org/data/ens-best-practices/styles.tsensawards.org/data/ens-best-practices/types.tsensawards.org/data/ens-best-practices/utils.tsensawards.org/src/components/atoms/badges/BenchmarkResultBadge.tsxensawards.org/src/components/atoms/badges/BenchmarkResultHeroBadge.astroensawards.org/src/components/atoms/cards/AppSummaryCard.tsxensawards.org/src/components/atoms/cards/BenchmarkSummaryCard.astroensawards.org/src/components/atoms/cards/BenchmarksPerAppTypeCard.tsxensawards.org/src/components/molecules/BenchmarkTechnicalDetails.tsxensawards.org/src/components/molecules/BestPracticeTechnicalDetails.tsxensawards.org/src/pages/app/[appSlug]/[categorySlug]/[bestPracticeSlug]/index.astroensawards.org/src/pages/app/[appSlug]/index.astro
Greptile SummaryThis PR refines the benchmark data model by introducing a new
Confidence Score: 5/5Safe to merge — a carefully structured data-model refactor with comprehensive test coverage and no regressions on existing score logic. The core data model change is well-contained, TypeScript-enforced, and all consuming code has been updated consistently. Score calculations are semantically correct for the current single-acceptance-test-per-best-practice constraint, and the test suite covers the main edge cases. ensawards.org/src/components/organisms/AppBestPracticeDetails.astro — the sort comparator accumulates differences by addition rather than returning on the first non-zero result; harmless today but worth cleaning up before adding multi-acceptance-test best practices. Important Files Changed
Reviews (6): Last reviewed commit: "Minor refactor -- resolving avoidable co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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 (2)
ensawards.org/data/benchmarks/utils.test.ts (1)
127-211:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDecouple
calcBestPracticeCategoryScoretests from prior mock state.These tests currently depend on
mockGetBestPracticeBySlugimplementations set in earliergroupBenchmarksByCategorytests. If this block runs in isolation, the mock returnsundefinedby default and failures become order-dependent.Suggested fix
describe("calcBestPracticeCategoryScore", () => { + const bestPracticeBySlug = { + "mock-best-practice-1": mockBestPractice1, + "mock-best-practice-2": mockBestPractice2, + "mock-best-practice-3": mockBestPractice3, + } as const; + + beforeEach(() => { + mockGetBestPracticeBySlug.mockReset(); + mockGetBestPracticeBySlug.mockImplementation( + (slug: BestPracticeSlug) => bestPracticeBySlug[slug as keyof typeof bestPracticeBySlug], + ); + }); + it("Should return undefined for no completed benchmarks", () => { ... }); it("Should return the rounded category score for valid benchmarks", () => { ... }); it("Should throw an error if benchmarks belong to different categories", () => { + mockGetBestPracticeBySlug.mockImplementation((slug: BestPracticeSlug) => { + if (slug === mockReverseResolutionBestPractice.bestPracticeSlug) { + return mockReverseResolutionBestPractice; + } + return bestPracticeBySlug[slug as keyof typeof bestPracticeBySlug]; + }); ... }); });🤖 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 `@ensawards.org/data/benchmarks/utils.test.ts` around lines 127 - 211, The tests for calcBestPracticeCategoryScore are relying on a previously configured mockGetBestPracticeBySlug; make them self-contained by stubbing mockGetBestPracticeBySlug at the top of this describe block to return the appropriate best-practice object for each slug used (e.g., return mockBestPractice1 for mockBestPractice1.bestPracticeSlug, mockBestPractice2 for mockBestPractice2.bestPracticeSlug, mockBestPractice3 for mockBestPractice3.bestPracticeSlug, and mockReverseResolutionBestPractice for its slug), and restore/reset the mock in an afterEach/beforeEach to avoid cross-test pollution; update the describe to explicitly set that mock so calcBestPracticeCategoryScore always receives valid best-practice data when run in isolation.ensawards.org/data/apps/utils.ts (1)
63-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInactive categories are still affecting the app score.
getAcceptanceTestBenchmarksByApp()flattens away the best-practice/category context, so this average still counts completed acceptance tests from hidden categories. That makes the hero score and any leaderboard ranking derived fromcalcAppScore()disagree with the visible UI, because bothAppSummaryCardand/src/pages/app/[appSlug]/index.astrofilter inactive categories out before rendering. Filter onbestPractice.category.status === CategoryStatuses.Activebefore flattening, or derive the score fromgetAppBenchmarks()instead so the category metadata is still available.Based on PR objectives, the Contract Naming category is being temporarily hidden and the score logic is expected to adjust with it.
🤖 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 `@ensawards.org/data/apps/utils.ts` around lines 63 - 78, calcAppScore is including benchmarks from inactive categories because it uses getAcceptanceTestBenchmarksByApp which loses category metadata; update calcAppScore to only include benchmarks whose bestPractice.category.status === CategoryStatuses.Active before computing totalPoints (or switch to deriving benchmarks from getAppBenchmarks() so you can filter by category status), then compute totalPoints via calcEnsAwardsPoints, round and return asEnsAwardsScore as before; ensure you reference getAcceptanceTestBenchmarksByApp, getAppBenchmarks, bestPractice.category.status, CategoryStatuses.Active, calcEnsAwardsPoints and asEnsAwardsScore when making the change.
🤖 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 `@ensawards.org/data/apps/utils.ts`:
- Around line 63-78: calcAppScore is including benchmarks from inactive
categories because it uses getAcceptanceTestBenchmarksByApp which loses category
metadata; update calcAppScore to only include benchmarks whose
bestPractice.category.status === CategoryStatuses.Active before computing
totalPoints (or switch to deriving benchmarks from getAppBenchmarks() so you can
filter by category status), then compute totalPoints via calcEnsAwardsPoints,
round and return asEnsAwardsScore as before; ensure you reference
getAcceptanceTestBenchmarksByApp, getAppBenchmarks,
bestPractice.category.status, CategoryStatuses.Active, calcEnsAwardsPoints and
asEnsAwardsScore when making the change.
In `@ensawards.org/data/benchmarks/utils.test.ts`:
- Around line 127-211: The tests for calcBestPracticeCategoryScore are relying
on a previously configured mockGetBestPracticeBySlug; make them self-contained
by stubbing mockGetBestPracticeBySlug at the top of this describe block to
return the appropriate best-practice object for each slug used (e.g., return
mockBestPractice1 for mockBestPractice1.bestPracticeSlug, mockBestPractice2 for
mockBestPractice2.bestPracticeSlug, mockBestPractice3 for
mockBestPractice3.bestPracticeSlug, and mockReverseResolutionBestPractice for
its slug), and restore/reset the mock in an afterEach/beforeEach to avoid
cross-test pollution; update the describe to explicitly set that mock so
calcBestPracticeCategoryScore always receives valid best-practice data when run
in isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 341b31fb-a82c-45d1-9d67-6573e97030c2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
ensawards.org/data/acceptance-tests/utils.tsensawards.org/data/apps/ambire-wallet/benchmarks/index.tsxensawards.org/data/apps/blockscout-explorer/benchmarks/index.tsxensawards.org/data/apps/coinbase-wallet/benchmarks/index.tsxensawards.org/data/apps/etherscan-explorer/benchmarks/index.tsxensawards.org/data/apps/metamask-wallet/benchmarks/index.tsxensawards.org/data/apps/rainbow-wallet/benchmarks/index.tsxensawards.org/data/apps/utils.test.tsensawards.org/data/apps/utils.tsensawards.org/data/apps/walletchan-wallet/benchmarks/index.tsxensawards.org/data/benchmarks/utils.test.tsensawards.org/data/benchmarks/utils.tsensawards.org/data/ens-best-practices/contract-naming/index.tsensawards.org/data/ens-best-practices/utils.tsensawards.org/src/components/atoms/cards/AppSummaryCard.tsxensawards.org/src/components/atoms/cards/ContributorsCard.tsxensawards.org/src/components/molecules/technicalDetails/benchmark/acceptanceTestResultDetails.tsxensawards.org/src/pages/app/[appSlug]/index.astro
Member
Author
tk-o
approved these changes
Jun 8, 2026
tk-o
left a comment
tk-o
left a comment
Member
There was a problem hiding this comment.
Left a couple of minor comments, otherwise, LGTM 🚀 I've checked the ENSAwards website preview and the updates look cool!
Member
Author
This was referenced Jun 8, 2026
This was referenced Jun 18, 2026
7 tasks
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
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.
Substantial PR → benchmark details refinement
Reviewer Focus (Read This First)
Problem & Motivation
What Changed (Concrete)
AcceptanceTests and their benchmarks (seeensawards.org/data/acceptance-tests/types.tsAppBenchmarks→AcceptanceTestBenchmarks) with appropriate JSDoc and unit test changes (seeensawards.org/data/benchmarks/types.ts)CONTRIBUTING.md)data/apps/*and the best practices' technical details indata/ens-best-practices/*/*.Design & Planning
This PR is loosely based on this spec
For the UI, displaying every acceptance test benchmark result separately was considered, but later decided against due to possible information overload for non-technical users
Planning artifacts: this Google Doc spec, multiple feedback docs, such as this one
Reviewed / approved by (only if there was a real review): Approved by @lightwalker-eth on the sync call on 05/07/26
Self-Review
ensawards.org/data/acceptance-tests/utils.test.tsand a JSDoc of a related function inensawards.org/data/acceptance-tests/utils.tsAcceptanceTestdata modelcalcAppsPassedfunction since it was unusedCross-Codebase Alignment
Recorddata structureDownstream & Consumer Impact
CONTRIBUTING.mddocsAcceptanceTest-related)Testing Evidence
typecheck,lint, andtestcommands locally to ensure that the migration didn't break anything, and later confirmed that in our CI workflowbest practices list,best practice details,benchmark details, andapp/protocol detailspagesScope Reductions
Risk Analysis
best practices list,best practice details,benchmark details, andapp/protocol detailspages on ENSAwardsbest practicessegment from the page or simplify the new data model as much as possiblePre-Review Checklist (Blocking)