feat: Add NotApplicable benchmark result type - #228
Conversation
…rk/feat/add-new-apps-exchanges
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds ChangesNotApplicable Benchmark Result and Score-Result Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new benchmark outcome, BenchmarkResults.NotApplicable, and threads it through scoring + UI so entities with only non-applicable benchmarks are excluded from scoring (similar to pending), while still being displayed with a distinct “Not Applicable” state.
Changes:
- Added
NotApplicableto benchmark/acceptance-test types and generalized-result formatting/sorting. - Updated app/best-practice/category scoring to exclude
NotApplicablebenchmarks and return anEnsAwardsScoreResult(score+ optional label for undefined scores). - Updated UI score components and benchmark badges/icons to display undefined-score labels and a dedicated Not Applicable indicator.
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ensawards.org/src/pages/index.astro | Adjusts leaderboard card prop shape for protocol scores. |
| ensawards.org/src/pages/app/[appSlug]/index.astro | Uses EnsAwardsScoreResult for app + category score display. |
| ensawards.org/src/components/organisms/AppBestPracticeDetails.astro | Updates score bar props and undefined-score styling hook. |
| ensawards.org/src/components/atoms/icons/AllBenchmarksNotApplicableIcon.tsx | Adds a UI icon for the “all benchmarks not applicable” state. |
| ensawards.org/src/components/atoms/ens-awards-score/circular.astro | Switches circular score wrapper to scoreResult input. |
| ensawards.org/src/components/atoms/ens-awards-score/circular-small.tsx | Updates small circular score to support undefined-score labels + NA icon. |
| ensawards.org/src/components/atoms/ens-awards-score/bar.tsx | Converts bar score UI to consume EnsAwardsScoreResult and display labels. |
| ensawards.org/src/components/atoms/ens-awards-score/bar-pending.tsx | Removes separate “pending bar” component (now handled via labels). |
| ensawards.org/src/components/atoms/CircularScore.astro | Updates core circular score component to accept scoreResult and display labels. |
| ensawards.org/src/components/atoms/cards/LeaderboardCard.astro | Changes leaderboard card score prop to EnsAwardsScoreResult. |
| ensawards.org/src/components/atoms/cards/BestPracticeCard.astro | Updates best-practice card to use ScoreBar with scoreResult. |
| ensawards.org/src/components/atoms/cards/AppSummaryCard.tsx | Wires app/category score displays to scoreResult props. |
| ensawards.org/src/components/atoms/badges/BenchmarkResultHeroBadge.astro | Adds hero badge rendering for NotApplicable. |
| ensawards.org/src/components/atoms/badges/BenchmarkResultBadge.tsx | Adds badge styles + icon mapping for NotApplicable. |
| ensawards.org/src/assets/benchmarkNotApplicableSymbol.svg | Adds NA symbol used by hero badge. |
| ensawards.org/data/shared/ens-awards-score.ts | Introduces EnsAwardsScoreResult and undefined-score label constants. |
| ensawards.org/data/ens-best-practices/utils.ts | Makes best-practice scoring exclude NotApplicable and return EnsAwardsScoreResult. |
| ensawards.org/data/ens-best-practices/utils.test.ts | Updates tests for new best-practice score result shape. |
| ensawards.org/data/benchmarks/utils.ts | Excludes NotApplicable in category scoring; formats/sorts NA benchmark results. |
| ensawards.org/data/benchmarks/utils.test.ts | Extends unit tests for NA scoring and sorting behavior. |
| ensawards.org/data/benchmarks/types.ts | Adds BenchmarkResults.NotApplicable. |
| ensawards.org/data/apps/worldapp-wallet/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/apps/utils.ts | Updates app score calc to return EnsAwardsScoreResult and exclude NA. |
| ensawards.org/data/apps/utils.test.ts | Updates app utils tests for NA-aware scoring/sorting. |
| ensawards.org/data/apps/robinhood-wallet/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/apps/robinhood-exchange/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/apps/readyx-wallet/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/apps/okx-exchange/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/apps/kraken-exchange/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/apps/cryptocom-exchange/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/apps/binance-wallet/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/apps/binance-exchange/benchmarks/index.tsx | Migrates benchmark(s) to NotApplicable. |
| ensawards.org/data/acceptance-tests/utils.ts | Updates benchmark generalization rules to recognize NotApplicable. |
| ensawards.org/data/acceptance-tests/utils.test.ts | Adds/updates tests for NA generalization cases. |
| ensawards.org/data/acceptance-tests/types.ts | Adds NotApplicableAcceptanceTestBenchmark and ApplicableAcceptanceTestBenchmark. |
| CONTRIBUTING.md | Documents new NotApplicable result and benchmark type additions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Suggestions:
- Type mismatch:
LeaderboardCardexpectsEnsAwardsScoreResult(object) but receivesEnsAwardsScore(number) in two files, causing build failure with "Either score or label must be defined in scoreResult. Both are undefined."
- Multiple components pass plain numbers (
EnsAwardsScore) where objects (EnsAwardsScoreResult) are expected, causing runtime invariant errors during build.
There was a problem hiding this comment.
⚠️ Not ready to approve
A newly added Tailwind class (bg-black/8) for NotApplicable badge styling is likely not generated by default Tailwind opacity scales and should be adjusted to a supported/arbitrary opacity value.
Copilot's findings
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 41/43 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Not ready to approve
calcBestPracticeScore currently assigns a non-type-guard .filter(...) result to ApplicableAcceptanceTestBenchmark[], which is type-unsafe and can break TypeScript typechecking.
Copilot's findings
- Files reviewed: 40/41 changed files
- Comments generated: 4
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Y3drk Thanks for these updates! Reviewed and shared feedback 👍
|
|
||
| /** | ||
| * Optional additional styles to apply to the pending score state. | ||
| * Optional additional styles to apply to the undefined score state. |
There was a problem hiding this comment.
@Y3drk Hey can you message me in Slack to help confirm what will be changing about the possible "pending" score state? Want to make sure we continue to support this possibility. "Pending" is different than "Not Applicable". "Not Applicable" requires that we actually performed the benchmark and then decided it was NA. However "Pending" means something different: that we haven't performed the benchmark at all yet.
| )} | ||
| > | ||
| <p className="text-muted-foreground text-sm leading-normal font-normal">ENSAwards score</p> | ||
| <p className="text-sm leading-normal font-medium text-black">Pending</p> |
There was a problem hiding this comment.
Worried that "Pending" is being removed?
There was a problem hiding this comment.
That component was unused either way. All the necessary logic is included in the atoms/ens-awards-score/bar.tsx
| label: EnsAwardsUndefinedScoreLabels.Pending, | ||
| }; | ||
|
|
||
| const completedApplicableAcceptanceTestBenchmarks: ApplicableAcceptanceTestBenchmark[] = |
There was a problem hiding this comment.
A "Not Applicable" benchmark is a benchmark that has been completed. You have to complete a benchmark to decide it is "Not Applicable".
Only "Pending" benchmarks haven't been completed yet.
There was a problem hiding this comment.
Yes, that's reflected in the refactored data model.
This list variable holds the benchmarks that are both completed (so all pending ones are filtered out) and applicable at the same time (so the completed but not-applicable ones are filtered out as well).
Appreciate the advice about the terminology, but overall, the logic you described above is implemented exactly like that.
| benchmark.result !== BenchmarkResults.NotApplicable, | ||
| ); | ||
|
|
||
| if (completedApplicableAcceptanceTestBenchmarks.length === 0) |
There was a problem hiding this comment.
Hmm, not sure if I'm understanding this correctly?
In my mind we never want to display "Not Applicable" in the context shown in this screenshot.
"Not Applicable" only applies to specific benchmarks, but here is an aggregate score for an entire app.
If all benchmarks for an app are pending and/or "Not Applicable" then the aggregate score for the entire app should reflect "Pending", not "Not Applicable".
This field for the overall score of an app should also never show "Not Applicable". In the case shown here it should show "Pending"
This field for the score of an app within a category should be able to support "Not Applicable". What is shown in this screenshot makes sense. There's only 1 best practice in the category. That one best practice is "Not Applicable" therefore the whole category is "Not Applicable".
However, for an overall app it should never be "Not Applicable". That sounds like we made a mistake to add the app to ENSAwards at all. For an overall app if the only ratings we have are "Pending" or "Not Applicable", the overall app score should still be "Pending".
There was a problem hiding this comment.
Applied a refactor so that these requirements are applied
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Y3drk Thanks for your updates. Looks good! 🫡
Lite PR → Add
NotApplicablebenchmark result typeSummary
BenchmarkResulttype →NotApplicable:EnsAwardsScorecalculating functions handle them the same way as thePendingbenchmarks (explicitly exclude them when counting the score, unless only this type of benchmark is available)EnsAwardsScoreResultthat should help in distinguishing between different outcomes of score calculation)NotApplicable(basically all apps that do not use ENS at all)Why
Testing
typecheck,lint, andtestcommands locally to ensure that the migration didn't break anything, and later confirmed that in our CI workflowNotes for Reviewer (Optional)
mainwithout any problems, but the current setup should make the review easier.Pre-Review Checklist (Blocking)