UI Overhaul + Code Optimization + Security Secrets - #26
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR establishes shared design tokens and UI components, adds lazy server clients and Valkey-backed caching, reduces hydration payloads, consolidates settings navigation, and refactors character, recommendation, team, and build-detail pages with expanded Playwright coverage. ChangesApplication overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
woopxwoop
changed the title
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/lib/ui/NavBar.svelte (1)
155-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMobile drawer won't close when switching Settings tabs.
All
settingsLinkspoint to/settings?tab=…, so navigating between them from an open drawer changes only the query string, notpathname. This effect trackspage.url.pathnamealone, somobileOpenstaystrueand the drawer keeps covering the page after a tab tap (main links close it because their pathname changes).🐛 Track the full URL so query-only navigations also close the drawer
$effect(() => { - page.url.pathname; + // Track the full URL so Settings tab switches (query-only nav) also close the drawer. + page.url.href; untrack(() => { if (mobileOpen) { closedByNavigation = true; mobileOpen = false; settingsDrawerExpanded = false; } }); });🤖 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/ui/NavBar.svelte` around lines 155 - 164, Update the navigation-tracking effect in NavBar.svelte to track the full page URL, including query parameters, instead of only page.url.pathname. Preserve the existing untracked drawer-closing logic so mobileOpen, closedByNavigation, and settingsDrawerExpanded are updated on any navigation, including Settings tab changes.
🤖 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/equipment-data.ts`:
- Around line 50-52: Update isFourStarWeapon to return true only when the weapon
exists and weapon.stars equals 4; preserve false for missing weapons and exclude
lower-star weapons from the R0 fallback.
In `@src/lib/server/cache.ts`:
- Around line 85-90: Update the getOrSet cache-miss path so the valkeySetJson
call for rKey is fire-and-forget rather than awaited, while preserving the
existing key, value, and TTL arguments and returning the computed value
immediately.
In `@src/lib/server/supabaseServer.ts`:
- Around line 32-41: Update the Reflect.get call in the serverDb proxy handler
to forward the resolved Supabase client as the receiver instead of the outer
proxy. Preserve the existing method binding behavior so function properties
remain bound to client.
In `@src/lib/server/valkey.ts`:
- Around line 33-52: Update the Valkey client configuration in getValkey’s
connection flow to set a short socket.connectTimeout and a capped
reconnectStrategy, ensuring unreachable hosts fail promptly and do not trigger
unbounded retries. Preserve the existing error handling and memory-cache
fallback behavior.
In `@src/lib/ui/components/BrowseFlipCard.svelte`:
- Around line 227-259: Update the focus-visible styling in BrowseFlipCard so
keyboard focus always provides a visual card-state indication on coarse-pointer
or hover:none devices. Keep hover flipping restricted to fine pointers, but add
an appropriate focus-visible flip or reduced-cue rule outside that restriction
while preserving the reduced-motion behavior.
In `@src/lib/ui/components/CharacterIcon.svelte`:
- Around line 145-150: Remove the unused object-position declaration from the
.icon-container-tcg img rule; preserve the existing transform-origin and
transform behavior, and do not add object-fit unless intentionally changing the
cropping behavior and updating the related comment.
In `@src/lib/ui/components/SegmentedControl.svelte`:
- Around line 22-38: Implement the keyboard and focus contract for the composite
controls: in src/lib/ui/components/SegmentedControl.svelte lines 22-38, add
radiogroup arrow-key navigation with roving tabindex, or replace the radio roles
with toggle-button semantics using aria-pressed; in
src/lib/ui/components/SlidingTabs.svelte lines 35-64, add tab arrow/Home/End
navigation, roving tabindex, and aria-controls relationships to associated
panels, or replace the tab roles with semantics matching a simple selector.
In `@src/lib/ui/components/Surface.svelte`:
- Around line 1-17: Update the Surface component’s props typing to use
HTMLAttributes<HTMLDivElement> from svelte/elements for the rest attributes
instead of the [key: string]: unknown index signature, applying the same Omit
pattern used by Button.svelte to exclude component-owned props. Preserve the
existing variant, flush, class, and children prop types while retaining
type-safe forwarding of valid div attributes.
In `@src/lib/ui/components/Team.svelte`:
- Line 20: In Team.svelte, remove the local compactIcons derivation and import
the shared isIconCompact store from stores.ts. Update the team slot class
binding to use $isIconCompact, preserving the existing compact styling behavior
through the centralized store.
In `@src/routes/abyss/`+page.svelte:
- Around line 227-335: Extract the duplicated solution-board and
meta-leaderboard markup and styles from src/routes/abyss/+page.svelte:227-335
and src/routes/stygian/+page.svelte:208-310 into shared components, such as
SolutionBoard and MetaLeaderboard. Parameterize the shared UI with each page’s
slot set, labels, slotRate, solutions, and per-slot render content, then update
both pages to use it; pass the 3-slot configuration and stygianSlotLabel from
Stygian. Ensure pager handling, slotRate mapping, behavior, and styling are
implemented once and remain identical across both pages.
In `@src/routes/stygian/`+page.svelte:
- Around line 90-94: Align all Stygian slot mappings with stygianSlotLabel and
the schema: map middle to Field 2/field_2_rate and bottom to Field
3/field_3_rate. Update slotRate, teamsOwnedStygianMiddle,
teamsOwnedStygianBottom, solver assignments, fieldScore, and leaderboard
ordering consistently, preserving top as Field 1.
---
Outside diff comments:
In `@src/lib/ui/NavBar.svelte`:
- Around line 155-164: Update the navigation-tracking effect in NavBar.svelte to
track the full page URL, including query parameters, instead of only
page.url.pathname. Preserve the existing untracked drawer-closing logic so
mobileOpen, closedByNavigation, and settingsDrawerExpanded are updated on any
navigation, including Settings tab changes.
🪄 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: c85d3ddd-4274-419c-b329-d5e78f72ef50
⛔ Files ignored due to path filters (16)
.github/workflows/deploy.ymlis excluded by none and included by nonedocker-compose.ymlis excluded by none and included by nonedockerfileis excluded by none and included by nonepackage.jsonis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by noneprometheus.ymlis excluded by none and included by nonesrc/lib/assets/favicon.svgis excluded by!**/*.svgand included bysrc/**static/adventurers-guild.pngis excluded by!**/*.pngand included by nonestatic/fonts/IBMPlexSans-Bold.woff2is excluded by!**/*.woff2and included by nonestatic/fonts/IBMPlexSans-Medium.woff2is excluded by!**/*.woff2and included by nonestatic/fonts/IBMPlexSans-Regular.woff2is excluded by!**/*.woff2and included by nonestatic/fonts/IBMPlexSans-SemiBold.woff2is excluded by!**/*.woff2and included by nonestatic/fonts/Manrope.ttfis excluded by!**/*.ttfand included by nonestatic/fonts/Manrope.woff2is excluded by!**/*.woff2and included by nonestatic/lightkeepers-mark.pngis excluded by!**/*.pngand included by nonesvelte.config.jsis excluded by none and included by none
📒 Files selected for processing (75)
src/app.csssrc/app.htmlsrc/hooks.server.tssrc/lib/app/bootstrapClient.tssrc/lib/build-stats.tssrc/lib/character-filter.tssrc/lib/data/character-bases.jsonsrc/lib/element-colors.tssrc/lib/equipment-data.tssrc/lib/is-new-character.tssrc/lib/server/auth.tssrc/lib/server/cache.tssrc/lib/server/supabaseServer.tssrc/lib/server/team-config.tssrc/lib/server/valkey.tssrc/lib/site.tssrc/lib/stores.tssrc/lib/ui/NavBar.sveltesrc/lib/ui/components/ArtifactTooltip.sveltesrc/lib/ui/components/Badge.sveltesrc/lib/ui/components/BrowseFlipCard.sveltesrc/lib/ui/components/Button.sveltesrc/lib/ui/components/CharacterFilterBar.sveltesrc/lib/ui/components/CharacterIcon.sveltesrc/lib/ui/components/CharacterPortraitCard.sveltesrc/lib/ui/components/CharacterTagSearch.sveltesrc/lib/ui/components/Chip.sveltesrc/lib/ui/components/EmptyState.sveltesrc/lib/ui/components/HoverTooltip.sveltesrc/lib/ui/components/LoadingState.sveltesrc/lib/ui/components/PageHeader.sveltesrc/lib/ui/components/PageShell.sveltesrc/lib/ui/components/SegmentedControl.sveltesrc/lib/ui/components/SlidingTabs.sveltesrc/lib/ui/components/SolutionDots.sveltesrc/lib/ui/components/Surface.sveltesrc/lib/ui/components/Team.sveltesrc/lib/ui/components/TeamCardHand.sveltesrc/lib/ui/components/Toggle.sveltesrc/lib/ui/components/WeaponTooltip.sveltesrc/lib/ui/pointer.tssrc/lib/utils.tssrc/routes/+layout.server.tssrc/routes/+layout.sveltesrc/routes/+layout.tssrc/routes/+page.sveltesrc/routes/abyss/+page.server.tssrc/routes/abyss/+page.sveltesrc/routes/abyss/+page.tssrc/routes/api/auth/[...all]/+server.tssrc/routes/api/static/+server.tssrc/routes/api/teams/+server.tssrc/routes/characters/+page.sveltesrc/routes/characters/[slug]/+page.sveltesrc/routes/dev/ui/+page.sveltesrc/routes/dev/ui/+page.tssrc/routes/pulls/+page.sveltesrc/routes/settings/+layout.sveltesrc/routes/settings/+page.sveltesrc/routes/settings/account/+page.sveltesrc/routes/settings/account/+page.tssrc/routes/settings/display/+page.tssrc/routes/settings/panels/AccountPanel.sveltesrc/routes/settings/panels/DisplayPanel.sveltesrc/routes/settings/panels/RosterPanel.sveltesrc/routes/settings/roster/+page.sveltesrc/routes/settings/roster/+page.tssrc/routes/stygian/+page.server.tssrc/routes/stygian/+page.sveltesrc/routes/stygian/+page.tssrc/routes/team-configs/+page.tssrc/routes/team-configs/[slug]/+page.server.tssrc/routes/team-configs/[slug]/+page.sveltesrc/routes/teams/+page.sveltesrc/routes/teams/[slug]/+page.svelte
💤 Files with no reviewable changes (5)
- src/routes/abyss/+page.ts
- src/routes/stygian/+page.ts
- src/routes/settings/account/+page.svelte
- src/routes/settings/roster/+page.svelte
- src/routes/+layout.ts
Comment on lines
+50
to
+52
| export function isFourStarWeapon(weaponKey: string): boolean { | ||
| const weapon = weaponByKey.get(weaponKey); | ||
| return weapon ? weapon.stars <= 4 : false; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict the R0 fallback to actual 4★ weapons.
<= 4 also classifies 3★ weapons as four-star, so hidden 3★ weapons incorrectly display as R0. Use weapon?.stars === 4.
Proposed fix
- return weapon ? weapon.stars <= 4 : false;
+ return weapon?.stars === 4;📝 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
| export function isFourStarWeapon(weaponKey: string): boolean { | |
| const weapon = weaponByKey.get(weaponKey); | |
| return weapon ? weapon.stars <= 4 : false; | |
| export function isFourStarWeapon(weaponKey: string): boolean { | |
| const weapon = weaponByKey.get(weaponKey); | |
| return weapon?.stars === 4; |
🤖 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/equipment-data.ts` around lines 50 - 52, Update isFourStarWeapon to
return true only when the weapon exists and weapon.stars equals 4; preserve
false for missing weapons and exclude lower-star weapons from the R0 fallback.
Comment on lines
+227
to
+335
| <PageShell class="gap-6"> | ||
| <header class="page-head"> | ||
| <div class="page-head-text"> | ||
| <h1 class="page-title">Spiral Abyss</h1> | ||
| {#if updatedLabel || abyssEnemies?.buffName} | ||
| <p class="page-meta"> | ||
| {#if abyssEnemies?.buffName} | ||
| <span title="Abyssal Moon blessing">{abyssEnemies.buffName}</span> | ||
| {/if} | ||
| {#if updatedLabel && abyssEnemies?.buffName} | ||
| <span class="page-meta-sep" aria-hidden="true">·</span> | ||
| {/if} | ||
| {#if updatedLabel} | ||
| <span>Updated {updatedLabel}</span> | ||
| {/if} | ||
| </p> | ||
| {/if} | ||
| </div> | ||
| {/if} | ||
| </header> | ||
|
|
||
| <!-- ── Side panels ────────────────────────────────────────────────────── --> | ||
| {#if loading} | ||
| <div class="flex items-center justify-center min-h-[40vh]"> | ||
| <p style="color: var(--foreground-mid);">Loading…</p> | ||
| </div> | ||
| <LoadingState /> | ||
| {:else} | ||
| <div class="grid lg:grid-cols-2 gap-4 items-start"> | ||
| {#each SLOTS as slot, slotIndex (slot)} | ||
| {#if isDesktop || slotIndex === activeSlotIndex} | ||
| <div | ||
| data-panel-slot={slot} | ||
| data-active-panel={slotIndex === activeSlotIndex} | ||
| <Surface flush class="solution-board"> | ||
| <div class="board-head"> | ||
| <div class="board-head-left"> | ||
| <span class="board-eyebrow"> | ||
| Solution {safeIndex + 1} | ||
| <span class="board-eyebrow-total">of {displaySolutions.length}</span> | ||
| </span> | ||
| </div> | ||
|
|
||
| <div class="board-actions"> | ||
| <button | ||
| type="button" | ||
| class="enemy-toggle" | ||
| aria-expanded={enemiesExpanded} | ||
| aria-controls="abyss-enemies-top abyss-enemies-bottom" | ||
| aria-label={enemiesExpanded ? "Hide enemies" : "Show enemies"} | ||
| onclick={() => (enemiesExpanded = !enemiesExpanded)} | ||
| > | ||
| <span>Enemies</span> | ||
| <IconChevronDown size={14} strokeWidth={2.25} /> | ||
| </button> | ||
|
|
||
| {#if displaySolutions.length > 1} | ||
| <div class="pager"> | ||
| <button | ||
| type="button" | ||
| class="pager-btn" | ||
| aria-label="Previous solution" | ||
| disabled={safeIndex === 0} | ||
| onclick={() => stepSolution(-1)} | ||
| > | ||
| <IconChevronDown size={16} strokeWidth={2.25} /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| class="pager-btn pager-btn-next" | ||
| aria-label="Next solution" | ||
| disabled={safeIndex === displaySolutions.length - 1} | ||
| onclick={() => stepSolution(1)} | ||
| > | ||
| <IconChevronDown size={16} strokeWidth={2.25} /> | ||
| </button> | ||
| </div> | ||
| {/if} | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="board-body"> | ||
| {#each SLOTS as slot (slot)} | ||
| <div class="board-cell" data-panel-slot={slot}> | ||
| {#key assignmentKey(slot)} | ||
| {@render slotPanel(slot)} | ||
| {@render halfColumn(slot)} | ||
| {/key} | ||
| </div> | ||
| {/if} | ||
| {/each} | ||
| </div> | ||
| {/each} | ||
| </div> | ||
| </Surface> | ||
|
|
||
| {#if solution?.isFallback && solution.neededCharacters.length > 0} | ||
| <p class="text-xs text-center" style="color: var(--foreground-mid);"> | ||
| Unable to find floor 12 clears with your roster - try teams similar to | ||
| those suggested or look at your pull suggestions | ||
| <p class="fallback-note"> | ||
| Unable to find floor 12 clears with your roster — try teams similar to | ||
| those suggested, or check Pull suggestions. | ||
| </p> | ||
| {/if} | ||
|
|
||
| <section class="meta-section"> | ||
| <header class="meta-head"> | ||
| <h2 class="meta-title">Meta teams</h2> | ||
| <p class="meta-lede"> | ||
| Teams with the highest | ||
| <span | ||
| class="meta-term" | ||
| title="usage rate × rate of appearing in this half" | ||
| >usage index</span | ||
| > in each side | ||
| </p> | ||
| </header> | ||
|
|
||
| <div class="meta-board"> | ||
| {#each SLOTS as slot (slot)} | ||
| {@render metaColumn(slot)} | ||
| {/each} | ||
| </div> | ||
| </section> | ||
| {/if} | ||
| </main> | ||
| </PageShell> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Extract the shared solution-board / meta-leaderboard UI to avoid abyss↔stygian drift. Both pages reimplement the same board header, pager, per-slot column, meta leaderboard, and nearly identical <style> block; they differ only in slot set, labels, solver call, and slotRate. The duplication is already diverging (pager event handling and slotRate mapping differ between the two), which is exactly the maintenance hazard consolidation prevents.
src/routes/abyss/+page.svelte#L227-L335: replace the inline board/meta markup + CSS with a shared component (e.g.SolutionBoard+MetaLeaderboard) parameterized by slots, labels,slotRate, and solutions.src/routes/stygian/+page.svelte#L208-L310: render the same shared component, passing the 3-slot config andstygianSlotLabel, so board/pager/meta behavior and styling stay identical to abyss.
📍 Affects 2 files
src/routes/abyss/+page.svelte#L227-L335(this comment)src/routes/stygian/+page.svelte#L208-L310
🤖 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/abyss/`+page.svelte around lines 227 - 335, Extract the duplicated
solution-board and meta-leaderboard markup and styles from
src/routes/abyss/+page.svelte:227-335 and
src/routes/stygian/+page.svelte:208-310 into shared components, such as
SolutionBoard and MetaLeaderboard. Parameterize the shared UI with each page’s
slot set, labels, slotRate, solutions, and per-slot render content, then update
both pages to use it; pass the 3-slot configuration and stygianSlotLabel from
Stygian. Ensure pager handling, slotRate mapping, behavior, and styling are
implemented once and remain identical across both pages.
Comment on lines
90
to
94
| function slotRate(team: StygianTeam, slot: Slot): number { | ||
| if (slot === "top") return team.field_1_rate ?? 0; | ||
| if (slot === "middle") return team.field_3_rate ?? 0; | ||
| return team.field_2_rate ?? 0; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm field_1_rate/field_2_rate/field_3_rate semantics and how they map to visual slots.
rg -nP 'field_[123]_rate' --type=ts -C2
fd -e ts 'database.types' --exec rg -nP 'field_[123]_rate' {}
# Inspect StygianTeam definition
rg -nP -C3 '\bStygianTeam\b' src/lib/definitions*Repository: woopxwoop/lightkeepers
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -u
echo "Files matching stygian/abyss and definitions:"
git ls-files | rg '(^src/routes/stygian/|^src/routes/abyss/|definitions|types|database\.types|styles)' | sed -n '1,200p'
echo
echo "Search for field_rate usages:"
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'field_[123]_rate|fieldScore|metaByField|bottom|middle|top.*rate|StygianTeam|AbyssTeam' . | sed -n '1,300p'Repository: woopxwoop/lightkeepers
Length of output: 17396
🏁 Script executed:
#!/bin/bash
set -u
echo "slottLabels:"
cat -n src/lib/slotLabels.ts
echo
echo "definitions:"
cat -n src/lib/definitions.ts | sed -n '1,120p'
echo
echo "stygian route relevant functions:"
cat -n src/routes/stygian/+page.svelte | sed -n '80,120p'
echo
echo "stores relevant derived fields:"
cat -n src/lib/stores.ts | sed -n '220,235p'
echo
echo "solver relevant constants/functions:"
cat -n src/lib/solver.ts | sed -n '1,80p;150,175p;210,230p;335,360p;480,510p'Repository: woopxwoop/lightkeepers
Length of output: 11436
Align Stygian field mappings before merge.
stygianSlotLabel says middle: "Field 2" and bottom: "Field 3", but slotRate, teamsOwnedStygianMiddle/teamsOwnedStygianBottom, the solver, fieldScore, and the leaderboard all map middle → field_3_rate and bottom → field_2_rate. Keep the label/schema semantics consistent so field-rate labels, ownership filters, assignments, and leaderboard ordering all use the same field ordering.
🤖 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/stygian/`+page.svelte around lines 90 - 94, Align all Stygian slot
mappings with stygianSlotLabel and the schema: map middle to Field
2/field_2_rate and bottom to Field 3/field_3_rate. Update slotRate,
teamsOwnedStygianMiddle, teamsOwnedStygianBottom, solver assignments,
fieldScore, and leaderboard ordering consistently, preserving top as Field 1.
…uccess” change): writeTeamsOwned / writeNearMissTeams swallowed errors → Pulls .catch never ran → stuck on “waiting” “Try again” only re-ranked empty stores — now invalidates and re-fetches
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/ui/NavBar.svelte (1)
43-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a path-segment boundary for prefix matches.
startsWith(link.path)marks routes such as/teams-previewas active for/teams. Match the exact path orpath + "/"instead.Proposed fix
function isMainActive(link: (typeof mainLinks)[number]): boolean { if (link.match === "exact") return page.url.pathname === link.path; - return page.url.pathname.startsWith(link.path); + return ( + page.url.pathname === link.path || + page.url.pathname.startsWith(`${link.path}/`) + ); }🤖 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/ui/NavBar.svelte` around lines 43 - 46, Update isMainActive so non-exact links match only when page.url.pathname equals link.path or begins with link.path followed by “/”. Preserve the existing exact-match behavior and prevent similarly prefixed paths such as /teams-preview from being marked active.src/lib/server/cache.ts (1)
90-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefer async work until its tracking promise is registered.
A synchronous exception before the first
awaitrunsfinallybefore the outer assignment completes, leavinginflightpermanently rejected orconnectingpermanently resolved tonull, which can block later retries. Defer the work so the shared async state is set beforefn()/client setup executes.
src/lib/server/cache.ts#L90-L101src/lib/server/valkey.ts#L34-59🤖 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/cache.ts` around lines 90 - 101, The async work in cache.ts around the inflight promise must be deferred until after this.inflight.set(key, pending) registers the shared promise, preventing synchronous fn() failures from running finally too early; apply the same deferral to the client setup flow in src/lib/server/valkey.ts lines 34-59 so connecting is registered before setup executes, with no direct change needed beyond this shared-state ordering fix.
🤖 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/stores.ts`:
- Around line 317-322: Consume the rejection from the cleanup promises created
by finally in the teams loading flow around writeTeamsOwned and in the near-miss
loading flow around nearMissInFlight. Update both src/lib/stores.ts sites at
lines 317-322 and 384-389 so the promise returned by finally is explicitly
handled, while preserving the existing in-flight identity checks and returned
original promises.
In `@src/lib/ui/components/SlidingTabs.svelte`:
- Around line 82-93: Update the tab buttons in SlidingTabs to establish an
accessible tab/panel relationship: add stable aria-controls values matching
corresponding panel IDs and ensure each controlled panel uses role="tabpanel"
with the matching ID. Preserve the existing role="tab" behavior, selection
state, and keyboard handling.
In `@tests/helpers.ts`:
- Around line 117-124: Update the ApiMockOptions type so abyssTeams,
stygianTeams, allTeamsAbyss, allTeamsStygian, nearMissTeams, and nearMissPairs
use the corresponding typed API endpoint item types instead of unknown[]. Apply
the existing endpoint payload types or satisfies-based validation so malformed
/api/teams, /api/nearmiss, and /api/static fixtures fail at compile time.
- Around line 126-130: Make SSR GET /api/static deterministic for E2E tests by
extending installApiMocks and the associated test setup to cover server-side
requests made during initial rendering of /abyss and /stygian, rather than only
Playwright browser routes. Add a server-side E2E fixture/test mode or
deterministic static fixture data used by src/lib/server/supabaseServer.ts,
ensuring SSR does not depend on Supabase secrets, cache state, or availability
while preserving existing client API mocks.
---
Outside diff comments:
In `@src/lib/server/cache.ts`:
- Around line 90-101: The async work in cache.ts around the inflight promise
must be deferred until after this.inflight.set(key, pending) registers the
shared promise, preventing synchronous fn() failures from running finally too
early; apply the same deferral to the client setup flow in
src/lib/server/valkey.ts lines 34-59 so connecting is registered before setup
executes, with no direct change needed beyond this shared-state ordering fix.
In `@src/lib/ui/NavBar.svelte`:
- Around line 43-46: Update isMainActive so non-exact links match only when
page.url.pathname equals link.path or begins with link.path followed by “/”.
Preserve the existing exact-match behavior and prevent similarly prefixed paths
such as /teams-preview from being marked active.
🪄 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: 2da4f73a-bcd9-4a64-8691-9ea42cd46002
⛔ Files ignored due to path filters (2)
.github/workflows/deploy.ymlis excluded by none and included by nonetsconfig.jsonis excluded by none and included by none
📒 Files selected for processing (23)
src/lib/equipment-data.tssrc/lib/is-new-character.tssrc/lib/server/auth.tssrc/lib/server/cache.tssrc/lib/server/supabaseServer.tssrc/lib/server/team-config.tssrc/lib/server/valkey.tssrc/lib/stores.tssrc/lib/ui/NavBar.sveltesrc/lib/ui/components/BrowseFlipCard.sveltesrc/lib/ui/components/CharacterIcon.sveltesrc/lib/ui/components/SegmentedControl.sveltesrc/lib/ui/components/SlidingTabs.sveltesrc/lib/ui/components/SolutionDots.sveltesrc/lib/ui/components/Surface.sveltesrc/lib/ui/components/Team.sveltesrc/routes/pulls/+page.sveltetests/abyss.spec.tstests/helpers.tstests/lazy-api.spec.tstests/pulls.spec.tstests/smoke.spec.tstests/stygian.spec.ts
💤 Files with no reviewable changes (1)
- src/lib/ui/components/CharacterIcon.svelte
Comment on lines
+82
to
+93
| {#each options as option, index (option.value)} | ||
| <button | ||
| type="button" | ||
| role="tab" | ||
| id="tab-{option.value}" | ||
| aria-selected={value === option.value} | ||
| tabindex={value === option.value ? 0 : -1} | ||
| onpointerdown={(event) => | ||
| handlePointerAction(event, () => (value = option.value))} | ||
| onclick={(event) => | ||
| handleKeyboardClick(event, () => (value = option.value))} | ||
| onkeydown={(event) => onTabKeydown(event, index)} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Associate each tab with its controlled panel.
These buttons retain role="tab", but no aria-controls points to a corresponding role="tabpanel". Either add a stable tab/panel ID contract, or use role="group" with aria-pressed like SegmentedControl.
🤖 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/ui/components/SlidingTabs.svelte` around lines 82 - 93, Update the
tab buttons in SlidingTabs to establish an accessible tab/panel relationship:
add stable aria-controls values matching corresponding panel IDs and ensure each
controlled panel uses role="tabpanel" with the matching ID. Preserve the
existing role="tab" behavior, selection state, and keyboard handling.
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/ui/NavBar.svelte (1)
49-51: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
onSettingsPagedoesn't get the same boundary fix asisMainActive.
isMainActivewas just hardened to require an exact match or a/-bounded prefix, butonSettingsPagestill uses a barestartsWith. No sibling route collides today, but this is inconsistent with the pattern just introduced a few lines above and drivesaria-current/drawer auto-expand.♻️ Suggested fix
- const onSettingsPage = $derived( - page.url.pathname.startsWith(resolve("/settings")), - ); + const onSettingsPage = $derived( + page.url.pathname === resolve("/settings") || + page.url.pathname.startsWith(`${resolve("/settings")}/`), + );🤖 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/ui/NavBar.svelte` around lines 49 - 51, Update the onSettingsPage derived expression to use the same exact-match or slash-bounded prefix logic as isMainActive, preserving correct aria-current behavior and drawer auto-expansion without matching similarly prefixed sibling routes.
🤖 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/ui/NavBar.svelte`:
- Around line 49-51: Update the onSettingsPage derived expression to use the
same exact-match or slash-bounded prefix logic as isMainActive, preserving
correct aria-current behavior and drawer auto-expansion without matching
similarly prefixed sibling routes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed43bf61-1c98-4209-b12a-74bc69425f0c
⛔ Files ignored due to path filters (4)
.github/workflows/playwright.ymlis excluded by none and included by nonepackage.jsonis excluded by none and included by noneplaywright.config.tsis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (24)
src/app.csssrc/lib/e2e/fixtures.tssrc/lib/server/cache.tssrc/lib/server/e2e.tssrc/lib/server/valkey.tssrc/lib/stores.tssrc/lib/ui/NavBar.sveltesrc/lib/ui/components/BrowseFlipCard.sveltesrc/lib/ui/components/CharacterIcon.sveltesrc/lib/ui/components/CharacterPortraitCard.sveltesrc/lib/ui/components/SlidingTabs.sveltesrc/routes/+layout.server.tssrc/routes/abyss/+page.sveltesrc/routes/api/nearmiss/+server.tssrc/routes/api/static/+server.tssrc/routes/api/teams/+server.tssrc/routes/characters/[slug]/+page.sveltesrc/routes/dev/ui/+page.sveltesrc/routes/pulls/+page.sveltesrc/routes/stygian/+page.sveltetests/abyss.spec.tstests/helpers.tstests/lazy-api.spec.tstests/smoke.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/debug.ts`:
- Around line 28-35: Update the requestfailed handler to log only the failed
request’s pathname rather than the full req.url(), removing query strings and
credentials; also normalize the optional req.failure()?.errorText to a
single-line value or omit it when unavailable. Preserve the existing request
method and failure logging context.
In `@tests/global-setup.ts`:
- Around line 9-15: Update the warmup requests in the paths.map callback to
apply a per-request abort timeout, ensuring hung routes terminate and
Promise.all can complete. Preserve the existing best-effort catch and separate
webServer readiness behavior.
🪄 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: 7bc4e332-7ab5-4371-9ace-ee149c2896b4
⛔ Files ignored due to path filters (2)
.github/workflows/playwright.ymlis excluded by none and included by noneplaywright.config.tsis excluded by none and included by none
📒 Files selected for processing (7)
src/routes/pulls/+page.sveltetests/abyss.spec.tstests/debug.tstests/global-setup.tstests/helpers.tstests/lazy-api.spec.tstests/pulls.spec.ts
This was referenced Jul 23, 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