GO: Add 4-liner computation, badge and filter#3177
Conversation
WalkthroughThis PR adds detection and filtering for artifacts that started with four visible substats ("4-liners"), exposes that state in schema and UI, renders a 4-liner badge on cards/tooltips, and includes unit tests and test utilities for the new logic. Changes
Sequence DiagramsequenceDiagram
actor User
participant FilterUI as ArtifactFilterDisplay
participant Util as hasFourInitialSubstats
participant Sort as ArtifactSortUtil
participant Card as ArtifactCard
User->>FilterUI: toggle initialSubstats
FilterUI->>FilterUI: update filterOption.initialSubstats
FilterUI->>Util: evaluate artifact (hasFourInitialSubstats)
Util->>Util: read cached rolls or artifact meta
Util-->>FilterUI: return boolean (fourLiner / notFourLiner)
FilterUI->>Sort: apply filter with initialSubstats key
Sort->>Sort: filter artifacts by key
Sort-->>Card: deliver filtered list
Card->>Card: render 4-liner badge when applicable
Card-->>User: display artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libs/gi/ui/src/components/artifact/ArtifactTooltip.tsx (1)
82-87: LGTM! Badge rendering is clean.The conditional 4-liner badge display is implemented nicely with the flex Box wrapper for proper spacing. The
color="info"gives it a distinct look from the level badge.One small thought: you've got a
badgeTooltiptranslation ("Started with 4 substats") in the localization file that isn't being used here. If you want to make the badge more accessible/informative, you could wrap the 4-liner SqBadge in a BootstrapTooltip:💡 Optional: Add tooltip for the badge
- {showFourLinerBadge && ( - <SqBadge color="info">{t('initialSubstats.fourLiner')}</SqBadge> - )} + {showFourLinerBadge && ( + <BootstrapTooltip title={t('initialSubstats.badgeTooltip')} placement="top"> + <SqBadge color="info">{t('initialSubstats.fourLiner')}</SqBadge> + </BootstrapTooltip> + )}But honestly, "4-Liner" is pretty self-explanatory to anyone who's pulled enough artifacts, so this is just a nice-to-have.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/gi/ui/src/components/artifact/ArtifactTooltip.tsx` around lines 82 - 87, Wrap the conditional four-liner SqBadge with a BootstrapTooltip (or the app's Tooltip component) and use the existing localization key (e.g., t('badgeTooltip') or t('initialSubstats.badgeTooltip')) for the tooltip content; update the JSX around showFourLinerBadge so the tooltip is only rendered when that badge is shown and keep the existing SqBadge color="info" and spacing inside Box so visual layout is unchanged (references: showFourLinerBadge, SqBadge, t).libs/gi/ui/src/components/artifact/ArtifactFilterDisplay.tsx (1)
138-140: Minor: Variable shadowing is intentional but worth noting.squints at variable names at 4 AM The local
const initialSubstatsat line 138 shadows the outerinitialSubstatsfrom line 100. This is intentional — the outer one is the filter option value (array of selected keys), while the inner one is the computed discriminator for each artifact ('fourLiner' or 'notFourLiner'). The scoping is correct, but if you ever revisit this code on low sleep (trust me, I know the feeling), consider renaming the inner one to something likeinitialSubstatsKeyorfourLinerStatusfor clarity.Not a blocker though — the existing pattern matches how
lock,excluded,equipped, etc. are handled in the same callback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/gi/ui/src/components/artifact/ArtifactFilterDisplay.tsx` around lines 138 - 140, The inner const initialSubstats in ArtifactFilterDisplay.tsx shadows the outer initialSubstats (the filter option array); rename the inner variable (e.g., initialSubstatsKey or fourLinerStatus) where it's assigned from hasFourInitialSubstats(art) and used in the artifact mapping callback so it no longer conflicts with the outer filter option; ensure all references in that callback (alongside lock, excluded, equipped handling) are updated to the new name to preserve behavior.libs/gi/util/src/artifact/artifact.test.ts (1)
14-33: Test helpers are well-designed.The
rollValueandmakeArtifacthelpers make the tests readable and maintainable. Just noting thatmakeArtifactin this file differs slightly from the one inartifactSortUtil.test.ts(this one doesn't takeidor use partial types). This is fine since they serve different purposes, but if you find yourself adding more artifact-related test files, consider extracting these to a shared test utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/gi/util/src/artifact/artifact.test.ts` around lines 14 - 33, The helper functions rollValue and makeArtifact are duplicated across tests; extract them into a shared test utility module (e.g., export rollValue and makeArtifact from a new test-utils file) and update tests that use them to import these helpers instead of redefining; ensure makeArtifact’s signature matches callers (add an optional id or accept Partial<IArtifact> if other tests need a different shape) and export the helpers so both artifact.test.ts and artifactSortUtil.test.ts can reuse them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/gi/ui/src/components/artifact/ArtifactFilterDisplay.tsx`:
- Around line 138-140: The inner const initialSubstats in
ArtifactFilterDisplay.tsx shadows the outer initialSubstats (the filter option
array); rename the inner variable (e.g., initialSubstatsKey or fourLinerStatus)
where it's assigned from hasFourInitialSubstats(art) and used in the artifact
mapping callback so it no longer conflicts with the outer filter option; ensure
all references in that callback (alongside lock, excluded, equipped handling)
are updated to the new name to preserve behavior.
In `@libs/gi/ui/src/components/artifact/ArtifactTooltip.tsx`:
- Around line 82-87: Wrap the conditional four-liner SqBadge with a
BootstrapTooltip (or the app's Tooltip component) and use the existing
localization key (e.g., t('badgeTooltip') or t('initialSubstats.badgeTooltip'))
for the tooltip content; update the JSX around showFourLinerBadge so the tooltip
is only rendered when that badge is shown and keep the existing SqBadge
color="info" and spacing inside Box so visual layout is unchanged (references:
showFourLinerBadge, SqBadge, t).
In `@libs/gi/util/src/artifact/artifact.test.ts`:
- Around line 14-33: The helper functions rollValue and makeArtifact are
duplicated across tests; extract them into a shared test utility module (e.g.,
export rollValue and makeArtifact from a new test-utils file) and update tests
that use them to import these helpers instead of redefining; ensure
makeArtifact’s signature matches callers (add an optional id or accept
Partial<IArtifact> if other tests need a different shape) and export the helpers
so both artifact.test.ts and artifactSortUtil.test.ts can reuse them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7daae43-f85c-4ee0-ba27-408b3129e953
📒 Files selected for processing (10)
libs/gi/localization/assets/locales/en/artifact.jsonlibs/gi/schema/src/filterOption.tslibs/gi/ui/src/components/artifact/ArtifactCard.tsxlibs/gi/ui/src/components/artifact/ArtifactFilterDisplay.tsxlibs/gi/ui/src/components/artifact/ArtifactTooltip.tsxlibs/gi/ui/src/components/artifact/editor/index.tsxlibs/gi/util/src/artifact/artifact.test.tslibs/gi/util/src/artifact/artifact.tslibs/gi/util/src/artifact/artifactSortUtil.test.tslibs/gi/util/src/artifact/artifactSortUtil.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/gi/ui/src/components/artifact/ArtifactFilterDisplay.tsx (1)
138-140: Consider performance:hasFourInitialSubstatscalled per artifact in bulk computation.yawns while scrolling through 1464 artifacts So this is called inside
useMemofor every artifact in the database. The function does lazy meta computation which helps, but for users with massive inventories (like my 2000+ artifact hoard from failing to get that one 5-star), this could add up.That said, the
useMemodependencies look correct and it'll only recompute whendatabase,filteredIdMap,filterOption, etc. change - not on every render. For most users this is probably fine.If performance becomes an issue, you could consider caching the 4-liner status on the artifact itself during database operations, but that's a bigger architectural change. For now this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/gi/ui/src/components/artifact/ArtifactFilterDisplay.tsx` around lines 138 - 140, The code calls hasFourInitialSubstats(art) for every artifact during the bulk useMemo computation which can be expensive for very large inventories; to fix, avoid repeated lazy meta work by computing that boolean once per artifact and caching it (either by adding a cached flag on the artifact model during DB load/updates, or maintain a local Map<artifactId, boolean> computed once before the loop), then replace direct calls to hasFourInitialSubstats in ArtifactFilterDisplay with lookups to that cache (reference the hasFourInitialSubstats function, the initialSubstatsKey assignment, and the useMemo that iterates artifacts to locate where to swap in the cached value).libs/gi/util/src/artifact/testUtils.ts (1)
13-24: Minor: Consider documenting the 3-star artifact edge case.blearily rubbing eyes So uh, I noticed while farming for that perfect artifact set I'll never get... this function handles
4 | 5explicitly as rarities, butArtifactRarityis actually3 | 4 | 5. If someone passes3, it gets treated as a roll index rather than rarity.For test utilities this is probably fine since GO mainly focuses on 5-star artifacts for optimization, and the docstring does clarify the behavior. But maybe add a note that 3-star isn't supported, or extend the check?
export function rollValue( key: SubstatKey, - rarityOrIndex: 4 | 5 | number, + rarityOrIndex: 3 | 4 | 5 | number, ...restIndices: number[] ) { const [rarity, indices]: [4 | 5, number[]] = - rarityOrIndex === 4 || rarityOrIndex === 5 + rarityOrIndex === 4 || rarityOrIndex === 5 || rarityOrIndex === 3 - ? [rarityOrIndex, restIndices] + ? [rarityOrIndex as 4 | 5, restIndices] // 3-star uses same structure : [5, [rarityOrIndex, ...restIndices]]Actually wait,
getSubstatValuesPercentacceptsArtifactRaritywhich includes 3... but the return type forces4 | 5. Might need to rethink this if 3-star tests are ever needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/gi/util/src/artifact/testUtils.ts` around lines 13 - 24, The rollValue function treats only rarities 4 and 5 (rarityOrIndex check) which misinterprets a 3 as an index; update rollValue to either explicitly support ArtifactRarity including 3 or clearly document that 3-star is unsupported: modify the rarity detection logic in rollValue (and adjust the tuple [rarity, indices]) to accept 3 | 4 | 5 when calling getSubstatValuesPercent, or add a docstring/comment above rollValue stating that 3-star artifacts are not handled and why, and ensure types reflect the chosen behavior (reference rollValue and getSubstatValuesPercent to align types and behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/gi/ui/src/components/artifact/ArtifactFilterDisplay.tsx`:
- Around line 138-140: The code calls hasFourInitialSubstats(art) for every
artifact during the bulk useMemo computation which can be expensive for very
large inventories; to fix, avoid repeated lazy meta work by computing that
boolean once per artifact and caching it (either by adding a cached flag on the
artifact model during DB load/updates, or maintain a local Map<artifactId,
boolean> computed once before the loop), then replace direct calls to
hasFourInitialSubstats in ArtifactFilterDisplay with lookups to that cache
(reference the hasFourInitialSubstats function, the initialSubstatsKey
assignment, and the useMemo that iterates artifacts to locate where to swap in
the cached value).
In `@libs/gi/util/src/artifact/testUtils.ts`:
- Around line 13-24: The rollValue function treats only rarities 4 and 5
(rarityOrIndex check) which misinterprets a 3 as an index; update rollValue to
either explicitly support ArtifactRarity including 3 or clearly document that
3-star is unsupported: modify the rarity detection logic in rollValue (and
adjust the tuple [rarity, indices]) to accept 3 | 4 | 5 when calling
getSubstatValuesPercent, or add a docstring/comment above rollValue stating that
3-star artifacts are not handled and why, and ensure types reflect the chosen
behavior (reference rollValue and getSubstatValuesPercent to align types and
behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d45d296b-2004-47bf-8b9e-357600be0cc1
📒 Files selected for processing (6)
libs/gi/ui/src/components/artifact/ArtifactFilterDisplay.tsxlibs/gi/ui/src/components/artifact/ArtifactTooltip.tsxlibs/gi/util/src/artifact/artifact.test.tslibs/gi/util/src/artifact/artifact.tslibs/gi/util/src/artifact/artifactSortUtil.test.tslibs/gi/util/src/artifact/testUtils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/gi/ui/src/components/artifact/ArtifactTooltip.tsx
- libs/gi/util/src/artifact/artifactSortUtil.test.ts
ChrisMGeo
changed the title
| const totalActiveRolls = artifact.substats.reduce((sum, substat, index) => { | ||
| if (!substat.key || !substat.value) return sum | ||
|
|
||
| const cachedRolls = (substat as typeof substat & { rolls?: number[] }).rolls |
There was a problem hiding this comment.
Let me know if there are better ways of doing this. It seemed to work for me, but I feel like I'm missing something like a condition to ensure it's cached or not.
1 participant
Describe your changes
/artifactsas well as theArtifactTooltipused inArtifactCardPicoandArtifactCardNanoIssue or discord link
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
New Features
Tests