Updated editor to support unactivated stats #3087
Conversation
WalkthroughImplements editing support for "unactivated" artifact substats: adds a localization string, a checkbox and local state in SubstatInput, extends setSubstat and messaging to carry an unactivated flag, and adds reducer handling and editor state/effects for unactivated substats. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SubstatInput
participant ArtifactEditor
participant Reducer
Note over SubstatInput: Toggle 4th substat as Unactivated and edit
User->>SubstatInput: Toggle "Unactivated" checkbox
SubstatInput->>SubstatInput: update local isUnactivatedSubstat
User->>SubstatInput: Edit/select 4th substat
SubstatInput->>ArtifactEditor: setSubstat(index, substat, isUnactivatedSubstat=true)
ArtifactEditor->>Reducer: dispatch { type: "unactivatedSubstat", index, substat }
Reducer->>Reducer: handle unactivatedSubstat (swap/reposition, update state)
Reducer-->>ArtifactEditor: return updated state
alt Regular substat edit
User->>SubstatInput: Edit/select substat
SubstatInput->>ArtifactEditor: setSubstat(index, substat, isUnactivatedSubstat=false)
ArtifactEditor->>Reducer: dispatch { type: "substat", index, substat }
Reducer->>Reducer: handle regular substat update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
|
[frontend] [Tue Oct 21 15:17:53 UTC 2025] - Deployed faf55a2 to https://genshin-optimizer-prs.github.io/pr/3087/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Oct 21 19:21:03 UTC 2025] - Deployed bc9e5dc to https://genshin-optimizer-prs.github.io/pr/3087/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Oct 21 20:24:22 UTC 2025] - Deployed 4464317 to https://genshin-optimizer-prs.github.io/pr/3087/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Oct 23 22:28:31 UTC 2025] - Deployed e4c702d to https://genshin-optimizer-prs.github.io/pr/3087/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Oct 25 19:15:20 UTC 2025] - Deployed 85f24db to https://genshin-optimizer-prs.github.io/pr/3087/frontend (Takes 3-5 minutes after this completes to be available) [Thu Nov 6 22:37:38 UTC 2025] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
libs/gi/ui/src/components/artifact/editor/index.tsx (1)
101-105: UnusedindexinUnactivatedSubstatMessage.Reducer ignores
action.index. Either remove it from the type and callers, or use it (future-proofing). Right now it’s confusing.Also applies to: 108-114
libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx (2)
41-55: Helper selection is fine; minor nit.Safe access is OK. If you want to be explicit, coalesce
artifact?.unactivatedSubstats?.[0] ?? artifact?.substats[substatIndex].
121-133: Guard effect to avoid re-enabling at high levels.At level ≥ 4 you set false, then can set true again if data still has an unactivated entry. Early-return to keep it off.
useEffect(() => { - if (level >= 4) { - setIsUnactivatedSubstat(false) - } - - if ( - artifact?.unactivatedSubstats?.length && - artifact.unactivatedSubstats[0].key - ) { - setIsUnactivatedSubstat(true) - } + if (level >= 4) { + setIsUnactivatedSubstat(false) + return + } + setIsUnactivatedSubstat(!!artifact?.unactivatedSubstats?.[0]?.key) }, [artifact?.unactivatedSubstats, level])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/gi/localization/assets/locales/en/artifact.json(1 hunks)libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx(9 hunks)libs/gi/ui/src/components/artifact/editor/index.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/gi/ui/src/components/artifact/editor/index.tsx (1)
libs/gi/good/src/IArtifact.ts (1)
IArtifact(11-16)
libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx (2)
libs/gi/db/src/Interfaces/ICachedArtifact.ts (1)
ICachedArtifact(2-7)libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx (1)
SubstatInput(42-262)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: call-build / build
- GitHub Check: typecheck
- GitHub Check: gen-file
- GitHub Check: format
- GitHub Check: call-build / build
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: call-build / build
🔇 Additional comments (4)
libs/gi/localization/assets/locales/en/artifact.json (1)
46-46: Add this key in other locales (and docs).Ensure editor.unactivated exists in non-EN locale files to avoid fallback/English bleed. Also add to translation checklist.
libs/gi/ui/src/components/artifact/editor/index.tsx (1)
340-343: Routing flag looks good; check other call sites.The new
setSubstat(index, substat, isUnactivatedSubstat)is wired here and in SubstatInput. Confirm no other callers exist.libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx (2)
64-69: Signature change propagates; verify consumers.Public prop now requires the boolean. Ensure no other SubstatInput usages (outside this editor) compile.
291-304: UX: Checkbox toggle behavior is good.Clearing the opposite side before toggling reduces desync. With the reducer init fix, first-time unactivated edits will work.
| case 'unactivatedSubstat': { | ||
| if (state?.unactivatedSubstats) { | ||
| const { substat } = action | ||
| const oldIndex = substat.key | ||
| ? state!.unactivatedSubstats.findIndex( | ||
| (current) => current.key === substat.key | ||
| ) | ||
| : -1 | ||
| if (oldIndex === -1 || oldIndex === 0) | ||
| state!.unactivatedSubstats[0] = substat | ||
| // Already in used, swap the items instead | ||
| else | ||
| [ | ||
| state!.unactivatedSubstats[0], | ||
| state!.unactivatedSubstats[oldIndex], | ||
| ] = [ | ||
| state!.unactivatedSubstats[oldIndex], | ||
| state!.unactivatedSubstats[0], | ||
| ] | ||
| } | ||
|
|
||
| return { ...state! } | ||
| } |
There was a problem hiding this comment.
First write to unactivatedSubstats is a no-op if array is undefined.
If the artifact doesn’t already have unactivatedSubstats, dispatching unactivatedSubstat silently does nothing. Initialize the array before use; keep swap logic.
Apply this diff inside the reducer case:
- case 'unactivatedSubstat': {
- if (state?.unactivatedSubstats) {
- const { substat } = action
- const oldIndex = substat.key
- ? state!.unactivatedSubstats.findIndex(
- (current) => current.key === substat.key
- )
- : -1
- if (oldIndex === -1 || oldIndex === 0)
- state!.unactivatedSubstats[0] = substat
- // Already in used, swap the items instead
- else
- [
- state!.unactivatedSubstats[0],
- state!.unactivatedSubstats[oldIndex],
- ] = [
- state!.unactivatedSubstats[oldIndex],
- state!.unactivatedSubstats[0],
- ]
- }
-
- return { ...state! }
- }
+ case 'unactivatedSubstat': {
+ const { substat } = action
+ // Ensure container exists
+ state!.unactivatedSubstats ??= []
+ const target = 0 // single unactivated slot supported
+ const oldIndex = substat.key
+ ? state!.unactivatedSubstats.findIndex(
+ (current) => current.key === substat.key
+ )
+ : -1
+ if (oldIndex === -1 || oldIndex === target)
+ state!.unactivatedSubstats[target] = substat
+ else
+ [state!.unactivatedSubstats[target], state!.unactivatedSubstats[oldIndex]] =
+ [state!.unactivatedSubstats[oldIndex], state!.unactivatedSubstats[target]]
+ return { ...state! }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 'unactivatedSubstat': { | |
| if (state?.unactivatedSubstats) { | |
| const { substat } = action | |
| const oldIndex = substat.key | |
| ? state!.unactivatedSubstats.findIndex( | |
| (current) => current.key === substat.key | |
| ) | |
| : -1 | |
| if (oldIndex === -1 || oldIndex === 0) | |
| state!.unactivatedSubstats[0] = substat | |
| // Already in used, swap the items instead | |
| else | |
| [ | |
| state!.unactivatedSubstats[0], | |
| state!.unactivatedSubstats[oldIndex], | |
| ] = [ | |
| state!.unactivatedSubstats[oldIndex], | |
| state!.unactivatedSubstats[0], | |
| ] | |
| } | |
| return { ...state! } | |
| } | |
| case 'unactivatedSubstat': { | |
| const { substat } = action | |
| // Ensure container exists | |
| state!.unactivatedSubstats ??= [] | |
| const target = 0 // single unactivated slot supported | |
| const oldIndex = substat.key | |
| ? state!.unactivatedSubstats.findIndex( | |
| (current) => current.key === substat.key | |
| ) | |
| : -1 | |
| if (oldIndex === -1 || oldIndex === target) | |
| state!.unactivatedSubstats[target] = substat | |
| else | |
| [state!.unactivatedSubstats[target], state!.unactivatedSubstats[oldIndex]] = | |
| [state!.unactivatedSubstats[oldIndex], state!.unactivatedSubstats[target]] | |
| return { ...state! } | |
| } |
🤖 Prompt for AI Agents
In libs/gi/ui/src/components/artifact/editor/index.tsx around lines 137 to 159,
the reducer case for 'unactivatedSubstat' silently no-ops when
state.unactivatedSubstats is undefined; initialize the array before using it: if
state.unactivatedSubstats is falsy, set it to a new array with the incoming
substat as the first item, otherwise run the existing index/find and swap logic
unchanged; ensure the function still returns a new state object at the end.
| disabled={ | ||
| (value && !rollNum) || | ||
| allowedRolls <= 0 || | ||
| (isUnactivatedSubstat && rollNum > 0 && index === 3) | ||
| } |
There was a problem hiding this comment.
Unactivated 4th substat still allows “next roll” buttons.
These buttons should be disabled when the substat is marked Unactivated; otherwise users can apply rolls to an unactivated slot.
- disabled={
- (value && !rollNum) ||
- allowedRolls <= 0 ||
- (isUnactivatedSubstat && rollNum > 0 && index === 3)
- }
+ disabled={
+ (value && !rollNum) ||
+ allowedRolls <= 0 ||
+ (isUnactivatedSubstat && index === 3)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| disabled={ | |
| (value && !rollNum) || | |
| allowedRolls <= 0 || | |
| (isUnactivatedSubstat && rollNum > 0 && index === 3) | |
| } | |
| disabled={ | |
| (value && !rollNum) || | |
| allowedRolls <= 0 || | |
| (isUnactivatedSubstat && index === 3) | |
| } |
🤖 Prompt for AI Agents
In libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx around lines 222
to 226, the "next roll" buttons still become enabled for an unactivated 4th
substat because the current condition only blocks some cases
(isUnactivatedSubstat && rollNum > 0 && index === 3) instead of blocking all
interactions when the substat is unactivated; change the disabled expression to
include isUnactivatedSubstat as a standalone OR term so that if
isUnactivatedSubstat is true the buttons are always disabled (i.e., add "||
isUnactivatedSubstat" to the existing boolean chain).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/gi/ui/src/components/artifact/editor/index.tsx (1)
145-177: Still silently no-ops when unactivatedSubstats is undefined (duplicate issue).This is the same issue from the previous review - if
state.unactivatedSubstatsdoesn't exist, the entire dispatch does nothing. You need to initialize the array. Also, there's a debugconsole.logon line 175 that should be removed before shipping. stares blearily at the screen at 4amApply this diff to fix the initialization issue and remove debug logging:
case 'unactivatedSubstat': { - if (state?.unactivatedSubstats) { - const { substat } = action + const { substat } = action + // Ensure container exists + state!.unactivatedSubstats ??= [] + + let activeSubstat = null - let activeSubstat = null - - if (state!.substats[3].key) { - activeSubstat = state!.substats[3] - state!.substats[3] = { key: '', value: 0 } - } else { - activeSubstat = substat - } - - const oldIndex = substat.key - ? state!.unactivatedSubstats.findIndex( - (current) => current.key === substat.key - ) - : -1 - if (oldIndex === -1 || oldIndex === 0) - state!.unactivatedSubstats[0] = activeSubstat - // Already in used, swap the items instead - else - [ - state!.unactivatedSubstats[0], - state!.unactivatedSubstats[oldIndex], - ] = [ - state!.unactivatedSubstats[oldIndex], - state!.unactivatedSubstats[0], - ] + if (state!.substats[3].key) { + activeSubstat = state!.substats[3] + state!.substats[3] = { key: '', value: 0 } + } else { + activeSubstat = substat } - console.log(state!.substats, 'subers') + + const oldIndex = substat.key + ? state!.unactivatedSubstats.findIndex( + (current) => current.key === substat.key + ) + : -1 + if (oldIndex === -1 || oldIndex === 0) + state!.unactivatedSubstats[0] = activeSubstat + // Already in used, swap the items instead + else + [ + state!.unactivatedSubstats[0], + state!.unactivatedSubstats[oldIndex], + ] = [ + state!.unactivatedSubstats[oldIndex], + state!.unactivatedSubstats[0], + ] + return { ...state! } }libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx (1)
222-226: Roll buttons still allow interaction with unactivated substats (duplicate issue).This is the same issue from the previous review - the condition
(isUnactivatedSubstat && rollNum > 0 && index === 3)means the buttons can still be enabled whenrollNum === 0for an unactivated substat. Should just be(isUnactivatedSubstat && index === 3)to block all rolls. facepalms while debugging gacha simulator at 5amApply this diff:
disabled={ (value && !rollNum) || allowedRolls <= 0 || - (isUnactivatedSubstat && rollNum > 0 && index === 3) + (isUnactivatedSubstat && index === 3) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx(9 hunks)libs/gi/ui/src/components/artifact/editor/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx (2)
libs/gi/db/src/Interfaces/ICachedArtifact.ts (1)
ICachedArtifact(2-7)libs/sr/ui/src/Relic/RelicEditor/SubstatInput.tsx (1)
SubstatInput(42-262)
libs/gi/ui/src/components/artifact/editor/index.tsx (1)
libs/gi/good/src/IArtifact.ts (1)
IArtifact(11-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: call-build / build
- GitHub Check: gen-file
- GitHub Check: typecheck
- GitHub Check: call-build / build
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (9)
libs/gi/ui/src/components/artifact/editor/index.tsx (3)
101-113: Type definitions look solid.The new
UnactivatedSubstatMessagetype and union extension are properly structured. yawns while scrolling through types at 3am
358-363: Clean API extension.The
setSubstatfunction now correctly routes to either 'substat' or 'unactivatedSubstat' based on the flag. Nice and simple, just how I like my code at 2am when my brain is mush from too many artifact rolls.
128-133: This behavior is intentional per the data model design.The codebase treats
unactivatedSubstatsas a temporary holding area for substats not yet revealed/activated. When you edit regular substats, clearing pending unactivated ones aligns with the pattern used elsewhere: the database validation (ArtifactDataManager.tslines 540-542, 468-470) also clearsunactivatedSubstatswhen substats get solidified. The data model explicitly states unactivated substats should move to activesubstatsonce activated.However, this is indeed an aggressive UX choice—any regular substat edit nukes pending unactivated substats. Worth documenting in comments if not already clear to future maintainers.
libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx (6)
26-26: Imports look good.Got all the checkbox bits you need. adds another energy drink to the pile
Also applies to: 34-34
41-55: Helper function logic is solid.Correctly routes to
unactivatedSubstats[0]for the 4th slot when unactivated mode is on, otherwise uses regular substats. My sleep-deprived brain appreciates the clarity.
64-68: Signature extension matches parent API.The type signature aligns with the changes in the parent component. Consistency maintained while I maintain my caffeine intake.
153-155: Consistent propagation of isUnactivatedSubstat flag.All the setSubstat calls correctly pass through the
isUnactivatedSubstatflag. No interaction paths left behind, unlike my sleep schedule.Also applies to: 167-173, 197-204, 246-250
204-204: Input/slider disabled logic is correct.Properly disables the value input and slider when the 4th substat is marked unactivated. Can't roll on unactivated stats, just like I can't roll decent artifacts after midnight.
Also applies to: 252-252
291-307: Checkbox UI implementation looks good.The unactivated toggle is correctly limited to the 4th substat only, with appropriate disabled conditions. The FormControlLabel and Checkbox setup is clean. Now if only my gacha luck was this clean...
| useEffect(() => { | ||
| if (level >= 4) { | ||
| setIsUnactivatedSubstat(false) | ||
| } | ||
|
|
||
| if ( | ||
| artifact?.unactivatedSubstats?.length && | ||
| artifact.unactivatedSubstats[0].key | ||
| ) { | ||
| setIsUnactivatedSubstat(true) | ||
| } | ||
| }, [artifact?.unactivatedSubstats, level]) |
There was a problem hiding this comment.
Inconsistent level check - useEffect vs checkbox disabled condition.
Line 122 resets isUnactivatedSubstat when level >= 4, but line 299 disables the checkbox when level > 0. These should match - if the checkbox is disabled at level 1+, the state should also reset at level 1+. squints at the discrepancy through tired eyes
Apply this diff to align the conditions:
useEffect(() => {
- if (level >= 4) {
+ if (level > 0) {
setIsUnactivatedSubstat(false)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (level >= 4) { | |
| setIsUnactivatedSubstat(false) | |
| } | |
| if ( | |
| artifact?.unactivatedSubstats?.length && | |
| artifact.unactivatedSubstats[0].key | |
| ) { | |
| setIsUnactivatedSubstat(true) | |
| } | |
| }, [artifact?.unactivatedSubstats, level]) | |
| useEffect(() => { | |
| if (level > 0) { | |
| setIsUnactivatedSubstat(false) | |
| } | |
| if ( | |
| artifact?.unactivatedSubstats?.length && | |
| artifact.unactivatedSubstats[0].key | |
| ) { | |
| setIsUnactivatedSubstat(true) | |
| } | |
| }, [artifact?.unactivatedSubstats, level]) |
🤖 Prompt for AI Agents
In libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx around lines 121
to 132, the useEffect resets isUnactivatedSubstat when level >= 4 but the
checkbox is disabled for level > 0; update the useEffect condition to match the
checkbox by resetting isUnactivatedSubstat when level > 0 (i.e., replace the
level >= 4 check with level > 0) and keep the subsequent
artifact.unactivatedSubstats check as-is so state reflects the actual checkbox
enabled/disabled logic.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
libs/gi/ui/src/components/artifact/editor/index.tsx (2)
146-151: First write to unactivatedSubstats is a no-op; initialize before use.When the array is undefined, this case returns early and drops the update. Initialize the container and ensure slot 0 exists. Same issue was flagged earlier.
- case 'unactivatedSubstat': { - if (!state?.unactivatedSubstats) { - return { ...state! } - } + case 'unactivatedSubstat': { + // Ensure container exists (IArtifact.unactivatedSubstats is optional) + state!.unactivatedSubstats ??= [] + if (state!.unactivatedSubstats.length === 0) + state!.unactivatedSubstats.push({ key: '', value: 0 })Based on learnings
523-527: Align level reset with checkbox disable (reset on any level > 0).The checkbox disables at level > 0 (SubstatInput), but the effect resets only at level >= 4. Make them consistent: reset when level > 0. This was called out previously.
- useEffect(() => { - if (level >= 4) { - setIsUnactivatedSubstat(false) - } - }, [artifact?.unactivatedSubstats, level]) + useEffect(() => { + if (level > 0) { + setIsUnactivatedSubstat(false) + } + }, [artifact?.unactivatedSubstats, level])libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx (1)
206-212: Disable “next roll” buttons for unactivated 4th substat in all cases.Current condition still enables buttons when rollNum === 0. Block them whenever the 4th substat is marked unactivated. This was raised earlier.
- disabled={ - (value && !rollNum) || - allowedRolls <= 0 || - (isUnactivatedSubstat && rollNum > 0 && index === 3) - } + disabled={ + (value && !rollNum) || + allowedRolls <= 0 || + (isUnactivatedSubstat && index === 3) + }
🧹 Nitpick comments (2)
libs/gi/ui/src/components/artifact/editor/index.tsx (2)
828-836: Pass the unactivated flag only to the 4th row (UI clarity, fewer surprises).Optional polish: keep the prop scoped to the 4th substat to avoid confusion downstream.
- <SubstatInput + <SubstatInput key={index} index={index} artifact={cArtifact} setSubstat={setSubstat} onChange={handleChange} - isUnactivatedSubstat={isUnactivatedSubstat} + isUnactivatedSubstat={index === 3 && isUnactivatedSubstat} />
380-387: Main-stat change should also evict matching unactivated substat (consistency).If the new main stat matches the unactivated substat key, clear the unactivated container to maintain invariants.
if (newValue.mainStatKey) { newValue.substats = [0, 1, 2, 3].map((i) => artifact && artifact.substats[i].key !== newValue.mainStatKey ? artifact!.substats[i] : { key: '', value: 0 } ) + if ( + artifact?.unactivatedSubstats?.[0]?.key === newValue.mainStatKey + ) { + newValue.unactivatedSubstats = [] + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx(8 hunks)libs/gi/ui/src/components/artifact/editor/index.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/gi/ui/src/components/artifact/editor/index.tsx (1)
libs/gi/good/src/IArtifact.ts (1)
IArtifact(11-16)
libs/gi/ui/src/components/artifact/editor/SubstatInput.tsx (1)
libs/gi/db/src/Interfaces/ICachedArtifact.ts (1)
ICachedArtifact(2-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: call-build / build
- GitHub Check: typecheck
- GitHub Check: gen-file
- GitHub Check: lint
- GitHub Check: test
| // Reset unactivated substat data if it exists | ||
| if ( | ||
| state!.unactivatedSubstats?.length && | ||
| state!.unactivatedSubstats[0].key | ||
| ) { | ||
| state!.unactivatedSubstats = [] | ||
| } | ||
| return { ...state! } |
There was a problem hiding this comment.
Don’t nuke unactivated data when editing other substats.
This clears unactivatedSubstats on any substat edit. Limit it to when the activated 4th slot is being edited (i.e., toggling back from unactivated).
- // Reset unactivated substat data if it exists
- if (
- state!.unactivatedSubstats?.length &&
- state!.unactivatedSubstats[0].key
- ) {
- state!.unactivatedSubstats = []
- }
+ // If editing the 4th slot in activated mode, clear the unactivated container
+ if (index === 3 && state!.unactivatedSubstats?.length) {
+ state!.unactivatedSubstats = []
+ }| const setSubstat = useCallback( | ||
| (index: number, substat: ISubstat) => { | ||
| artifactDispatch({ type: 'substat', index, substat }) | ||
| (index: number, substat: ISubstat, isUnactivatedSubstat: boolean) => { | ||
| const type = isUnactivatedSubstat ? 'unactivatedSubstat' : 'substat' | ||
| artifactDispatch({ type: type, index, substat }) | ||
| }, |
There was a problem hiding this comment.
Gate unactivated handling to the 4th substat only (prevents cross-row mutations).
Right now, any row edit while the toggle is on dispatches as “unactivated”, which explains the “editing 3rd changes 4th” reports. Only the 4th slot can be unactivated—gate by index.
- const setSubstat = useCallback(
- (index: number, substat: ISubstat, isUnactivatedSubstat: boolean) => {
- const type = isUnactivatedSubstat ? 'unactivatedSubstat' : 'substat'
- artifactDispatch({ type: type, index, substat })
- },
+ const setSubstat = useCallback(
+ (index: number, substat: ISubstat, isUnactivatedSubstat: boolean) => {
+ const type =
+ isUnactivatedSubstat && index === 3 ? 'unactivatedSubstat' : 'substat'
+ artifactDispatch({ type, index, substat })
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setSubstat = useCallback( | |
| (index: number, substat: ISubstat) => { | |
| artifactDispatch({ type: 'substat', index, substat }) | |
| (index: number, substat: ISubstat, isUnactivatedSubstat: boolean) => { | |
| const type = isUnactivatedSubstat ? 'unactivatedSubstat' : 'substat' | |
| artifactDispatch({ type: type, index, substat }) | |
| }, | |
| const setSubstat = useCallback( | |
| (index: number, substat: ISubstat, isUnactivatedSubstat: boolean) => { | |
| const type = | |
| isUnactivatedSubstat && index === 3 ? 'unactivatedSubstat' : 'substat' | |
| artifactDispatch({ type, index, substat }) | |
| }, |
🤖 Prompt for AI Agents
In libs/gi/ui/src/components/artifact/editor/index.tsx around lines 391-395,
update setSubstat so that the "unactivatedSubstat" branch is only used for the
4th slot (index === 3); if isUnactivatedSubstat is true but index !== 3, treat
it as a normal 'substat' to avoid cross-row mutations. Change the
ternary/branching that picks the dispatch type to check index === 3 in addition
to isUnactivatedSubstat, and dispatch the appropriate action accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
libs/gi/ui/src/components/artifact/editor/index.tsx (3)
147-149: Critical: Still not initializing the unactivated array on first userubs tired eyes This is the same issue from the last review... When
unactivatedSubstatsis undefined, we just bail out and return the state unchanged. So the first time someone tries to use this feature? Nothing happens. Gacha games taught me that failing silently is the worst kind of failure...Initialize the array before attempting to use it, as suggested in the previous review.
137-143: Major: Still nuking unactivated data on every substat editsighs while rolling another 3-star This code wipes the unactivated substats whenever ANY substat is edited, not just the 4th slot. If I spent time setting up my unactivated 4th stat and then tweaked my 2nd substat? Poof, gone. That's worse than losing a 50/50...
Only clear when editing the 4th slot specifically (index === 3), as suggested in the previous review.
392-398: Critical: Unactivated dispatches aren't gated to the 4th substatmumbles incoherently after another all-nighter Right now, when that checkbox is on, editing ANY row sends an 'unactivatedSubstat' action. That's why we're seeing "edit 3rd row, 4th row changes" bug reports. It's like trying to pull for one character but the gacha gives you a different one...
Only dispatch 'unactivatedSubstat' when
index === 3, as the previous review suggested. Otherwise treat it as a normal 'substat' action.
🧹 Nitpick comments (1)
libs/gi/ui/src/components/artifact/editor/index.tsx (1)
524-532: Optional: Refine the dependency array for better precisionyawns while farming artifacts at 2am The effect depends on
artifact?.unactivatedSubstatsbut only checksartifact?.unactivatedSubstats?.[0]?.keyinside. This means if the array reference changes but the first element's key stays the same, we still re-run. Not critical, but addingartifact?.unactivatedSubstats?.[0]?.keyto deps instead would be more precise. Like optimizing your artifact farming route instead of just teleporting randomly...useEffect(() => { if (level >= 4) { setIsUnactivatedSubstat(false) } if (artifactIdToEdit && artifact?.unactivatedSubstats?.[0]?.key) { setIsUnactivatedSubstat(true) } - }, [artifact?.unactivatedSubstats, artifactIdToEdit, level]) + }, [artifact?.unactivatedSubstats?.[0]?.key, artifactIdToEdit, level])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/gi/ui/src/components/artifact/editor/index.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/gi/ui/src/components/artifact/editor/index.tsx (1)
libs/gi/good/src/IArtifact.ts (1)
IArtifact(11-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: call-build / build
- GitHub Check: typecheck
- GitHub Check: gen-file
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (2)
libs/gi/ui/src/components/artifact/editor/index.tsx (2)
101-113: Type definitions look solid!yawns The new
UnactivatedSubstatMessagetype is structured correctly and properly added to theMessageunion. At least the types won't betray us at 3am...
833-842: Observation: All rows share the same unactivated statestruggles to keep eyes open All four SubstatInput components get the same
isUnactivatedSubstatstate andonChangehandler. This means the checkbox state and behavior are global, not per-row. While this might be intentional for the UI (only one row can be unactivated at a time), it compounds the cross-row mutation issues mentioned in other comments. Once the gating insetSubstatandhandleChangeis fixed to restrict unactivated handling to index 3, this shared state should work correctly.
| const handleChange = ( | ||
| index: number, | ||
| substat: ISubstat, | ||
| isChecked: boolean | ||
| ) => { | ||
| setIsUnactivatedSubstat(isChecked) | ||
| setSubstat(index, substat, isChecked) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Suggested refactor: handleChange should gate checkbox logic to row 4 only
stares blankly at screen at 4am This function sets the global isUnactivatedSubstat state without checking which row we're on. Since the checkbox can appear on any row (based on how SubstatInput is called), checking it on row 1, 2, or 3 would affect behavior globally. Combined with the ungated dispatch in setSubstat, this creates the cross-row mutation issue.
Consider adding a guard: only set the unactivated state when index === 3, like this:
const handleChange = (
index: number,
substat: ISubstat,
isChecked: boolean
) => {
- setIsUnactivatedSubstat(isChecked)
+ if (index === 3) {
+ setIsUnactivatedSubstat(isChecked)
+ }
setSubstat(index, substat, isChecked)
}This pairs with the fix needed in setSubstat to fully prevent cross-row issues.
🤖 Prompt for AI Agents
In libs/gi/ui/src/components/artifact/editor/index.tsx around lines 515 to 522,
handleChange currently sets the global isUnactivatedSubstat and calls setSubstat
for every row; restrict that behavior by only calling setIsUnactivatedSubstat
when index === 3 (the fourth row), and ensure setSubstat likewise guards its
dispatch/logic so it does not apply unactivated-checkbox behavior for other
indices; update handleChange to gate setIsUnactivatedSubstat to index === 3 and
modify setSubstat to check the index before applying any
cross-row/unactivated-specific mutations.
2 participants
Describe your changes
Issue 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
Localization