add main functionality + ui for characters page - #24
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a CDN-backed Characters experience with searchable roster and detail pages, shared character and enemy contracts, cached investment loading, expanded weapon data, and new team filtering and baseline comparison presentations. ChangesCharacter and investment experiences
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CharacterPage
participant getCharacterKit
participant getCharacterSummary
participant CDN
CharacterPage->>getCharacterKit: request kit by slug
getCharacterKit->>CDN: fetch character kit JSON
CharacterPage->>getCharacterSummary: request builds by goodKey
getCharacterSummary->>CDN: fetch character summary JSON
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@src/lib/asset-urls.ts`:
- Around line 140-176: Add an explicit timeout/abort signal to the fetch calls
in fetchCharacterKitIndex, fetchCharacterKit, and fetchEnemyIndex, using the
project’s established timeout value or CDN fetch convention. Preserve the
existing response validation, error messages, and JSON parsing behavior while
ensuring stalled CDN requests terminate within a bounded period.
In `@src/lib/data/weapons.json`:
- Around line 1025-1027: Update the id 11428 weapon entry’s awakenIcon value to
use the awakened texture suffix, setting it to
UI_EquipIcon_Sword_Purewill_Awaken while leaving icon and splashIcon unchanged.
In `@src/lib/server/character-kit.ts`:
- Around line 18-21: Add an AbortController-based timeout to the fetch in
getCharacterKit, passing its signal to the CDN request and ensuring the timer is
cleared after completion. Treat an abort or timeout like other fetch failures by
returning null, while preserving the existing successful-response and !res.ok
behavior.
- Around line 14-28: Update the cache flow in the character-kit loader to cache
negative results as null, matching the behavior of character-summary.ts. Store
null in kitCache for non-OK responses and caught fetch/JSON errors, while
preserving cached successful CharacterKit values and returning them unchanged.
In `@src/lib/server/character-summary.ts`:
- Around line 20-24: Update the fetch flow in getCharacterSummary to use an
AbortController with a timeout, passing its signal to fetch and ensuring the
timeout is cleared after completion. Preserve the existing non-OK response
behavior that caches null and returns null.
In `@src/lib/types/investment.ts`:
- Around line 93-124: Update the documentation comment above CharacterIndexFile
to describe only the output/characters.json shape, removing the misleading
reference to per-key characters/{GoodKey}.json files; leave the CharacterIndex
interface and its fields unchanged.
In `@src/lib/utils.ts`:
- Around line 37-56: Update resolveGameLinks to validate the href returned by
resolveLink before generating the anchor, allowing only safe internal targets
that begin with “#” or “/”. Treat any other scheme or invalid href as unresolved
and return the original inner text, while preserving empty-link removal and
valid-link rendering.
In `@src/routes/characters/`[slug]/+page.svelte:
- Around line 47-56: Remove event.preventDefault() from handlePointerAction and
the corresponding pointer handlers for both tab buttons, allowing pointer
activation to preserve the browser’s default focus behavior and focus ring.
In `@src/routes/characters/`+page.svelte:
- Around line 88-94: Update the matchesWeapon expression to reuse the existing
weaponTypeLabel helper for normalized weapon-type values instead of duplicating
the WEAPON_TYPE_MAP lookup and fallback logic. Preserve the current null check
and weaponFilter behavior, and reference the imported weaponTypeLabel used
elsewhere in the file.
In `@src/routes/teams/`[slug]/+page.svelte:
- Line 256: Update the baseline weapon card’s refinement value around the
{`@const` refine} declaration and the related rendering at lines 277–288 to use
displayWeaponRefinement, matching the character and compact card formatting.
Preserve the R0 convention for all 4★ weapons regardless of stored refinement
values.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f9707eb-1989-4d3d-bf63-6b3f125dde9a
⛔ Files ignored due to path filters (1)
package.jsonis excluded by none and included by none
📒 Files selected for processing (25)
src/lib/app/bootstrapClient.tssrc/lib/app/investment.tssrc/lib/asset-urls.tssrc/lib/data/weapons.jsonsrc/lib/definitions.tssrc/lib/server/character-kit.tssrc/lib/server/character-summary.tssrc/lib/types/character-kit.tssrc/lib/types/enemy.tssrc/lib/types/investment.tssrc/lib/types/schedules.tssrc/lib/ui/NavBar.sveltesrc/lib/ui/components/ArtifactTooltip.sveltesrc/lib/ui/components/GameText.sveltesrc/lib/ui/components/HoverTooltip.sveltesrc/lib/ui/components/WeaponTooltip.sveltesrc/lib/utils.tssrc/routes/+page.sveltesrc/routes/api/static/+server.tssrc/routes/characters/+page.sveltesrc/routes/characters/+page.tssrc/routes/characters/[slug]/+page.server.tssrc/routes/characters/[slug]/+page.sveltesrc/routes/teams/+page.sveltesrc/routes/teams/[slug]/+page.svelte
Comment on lines
1025
to
+1027
| "icon": "UI_EquipIcon_Sword_Purewill", | ||
| "awakenIcon": "UI_EquipIcon_Sword_Purewill", | ||
| "splashIcon": "UI_Gacha_EquipIcon_Sword_Purewill" | ||
| "splashIcon": "UI_Gacha_EquipIcon_Sword_Purewill", |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
awakenIcon is missing the _Awaken suffix.
For id 11428 (Sword of Narzissenkreuz) awakenIcon is "UI_EquipIcon_Sword_Purewill", identical to icon and unlike every other entry which uses ..._Awaken. This will resolve the awakened variant to the base texture. Confirm the source data and set it to "UI_EquipIcon_Sword_Purewill_Awaken" if that's the intended stem.
🔧 Proposed fix
"icon": "UI_EquipIcon_Sword_Purewill",
- "awakenIcon": "UI_EquipIcon_Sword_Purewill",
+ "awakenIcon": "UI_EquipIcon_Sword_Purewill_Awaken",
"splashIcon": "UI_Gacha_EquipIcon_Sword_Purewill",📝 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.
Suggested change
| "icon": "UI_EquipIcon_Sword_Purewill", | |
| "awakenIcon": "UI_EquipIcon_Sword_Purewill", | |
| "splashIcon": "UI_Gacha_EquipIcon_Sword_Purewill" | |
| "splashIcon": "UI_Gacha_EquipIcon_Sword_Purewill", | |
| "icon": "UI_EquipIcon_Sword_Purewill", | |
| "awakenIcon": "UI_EquipIcon_Sword_Purewill_Awaken", | |
| "splashIcon": "UI_Gacha_EquipIcon_Sword_Purewill", |
🤖 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 `@src/lib/data/weapons.json` around lines 1025 - 1027, Update the id 11428
weapon entry’s awakenIcon value to use the awakened texture suffix, setting it
to UI_EquipIcon_Sword_Purewill_Awaken while leaving icon and splashIcon
unchanged.
| ? weaponByKey.get(baselineBuild.weapon.key) | ||
| : null} | ||
| {@const icon = weapon ? weaponIconUrl(weapon.awakenIcon) : null} | ||
| {@const refine = baselineBuild?.weapon.refinement ?? null} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Baseline weapon card shows raw refinement — 4★ weapons won't render R0.
Unlike the character/compact cards (which go through formatInvestmentCR → displayWeaponRefinement), this badge renders the raw baselineBuild.weapon.refinement as R{refine}. For 4★ weapons this displays the stored refinement instead of the app's R0 convention, so this card disagrees with every other refinement label.
Normalize via displayWeaponRefinement for consistency:
🐛 Proposed fix
import {
buildGoodKeyMap,
toGoodKey,
getSimConfigUrl,
humanizeInvestmentLabel,
weaponByKey,
+ displayWeaponRefinement,
} from "$lib/utils";- {`@const` refine = baselineBuild?.weapon.refinement ?? null}
+ {`@const` refine = baselineBuild
+ ? displayWeaponRefinement(
+ baselineBuild.weapon.key,
+ baselineBuild.weapon.refinement,
+ )
+ : null}Also applies to: 277-288
🤖 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 `@src/routes/teams/`[slug]/+page.svelte at line 256, Update the baseline weapon
card’s refinement value around the {`@const` refine} declaration and the related
rendering at lines 277–288 to use displayWeaponRefinement, matching the
character and compact card formatting. Preserve the R0 convention for all 4★
weapons regardless of stored refinement values.
Source: Learnings
Contributor
There was a problem hiding this comment.
@woopxwoop Understood — I’ll leave the baseline weapon refinement display as-is.
🐇 …
🧠 Learnings used
Learnt from: woopxwoop
Repo: woopxwoop/lightkeepers PR: 22
File: src/routes/teams/+page.svelte:209-225
Timestamp: 2026-07-16T03:02:42.127Z
Learning: In `src/routes/teams/**/+page.svelte`, the `formatCR` function may intentionally display “R0” for all 4-star weapons even when the underlying stored `refinement` value differs. This follows the community convention that 4-star weapon refinement ranks are not meaningful. In future reviews, do not flag this as a bug if the UI renders “R0” for 4-star weapons by design (i.e., based on the star rating) rather than reflecting the raw `refinement` value.
Contributor
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 (1)
src/lib/server/character-summary.ts (1)
30-32: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCache
nullon fetch exceptions to prevent thundering herds.Unlike
character-kit.ts, thiscatchblock returnsnullwithout caching the failure. If the CDN times out or dns resolution fails, every subsequent request for this character will trigger another 10-second fetch rather than hitting the cache, heavily tying up server connection pools.🔒️ Proposed fix to cache the failure state
} catch { + summaryCache.set(goodKey, null); return null; }🤖 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 `@src/lib/server/character-summary.ts` around lines 30 - 32, Update the exception path in the character summary fetch function so the caught fetch failure stores a null result in the same cache used for successful summaries before returning null. Match the failure-caching behavior of character-kit.ts and preserve the existing return value.
🤖 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 `@src/lib/server/character-summary.ts`:
- Around line 30-32: Update the exception path in the character summary fetch
function so the caught fetch failure stores a null result in the same cache used
for successful summaries before returning null. Match the failure-caching
behavior of character-kit.ts and preserve the existing return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d9643efa-d94b-43bb-a827-8c69740fbb8d
⛔ Files ignored due to path filters (3)
package.jsonis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by nonepnpm-workspace.yamlis excluded by none and included by none
📒 Files selected for processing (10)
src/lib/asset-urls.tssrc/lib/cdn-fetch.tssrc/lib/data/artifact-sets.jsonsrc/lib/data/artifacts.jsonsrc/lib/server/character-kit.tssrc/lib/server/character-summary.tssrc/lib/types/investment.tssrc/lib/utils.tssrc/routes/characters/+page.sveltesrc/routes/characters/[slug]/+page.svelte
💤 Files with no reviewable changes (1)
- src/routes/characters/[slug]/+page.svelte
woopxwoop
changed the title
This was referenced Jul 20, 2026
This was referenced Jul 29, 2026
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
1 participant
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.
Summary by CodeRabbit