Conversation
Stat goals
Stygian videos
|
Warning Review limit reached
Next review available in: 90 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (15)
📝 WalkthroughWalkthroughThis change adds Stygian clear-solving modes, cost and video APIs, client caching, and a reusable solution board. It also adds character build-example contracts, stat-roll clamping, build conversion helpers, character-page presentation, investment-label options, and a development stat-goal gallery. ChangesStygian solving and clear data
Character build examples
Presentation refinements and development studies
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🟡 Moderate · up to This PR adds Stygian recommendations and simulated-character build goals, but the current head can show incorrect build-stat tiers, omit usable partial recommendations, and display misleading fallback guidance. These concrete user-facing correctness issues should be fixed or explicitly accepted before merging. Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Player
participant StygianSolutionBoard
participant Solver
participant ClearCache
participant ClearAPI
Player->>StygianSolutionBoard: select solver mode and difficulty
StygianSolutionBoard->>ClearCache: request clear rows or videos
ClearCache->>ClearAPI: fetch missing normalized data
ClearAPI-->>ClearCache: return validated clear data
StygianSolutionBoard->>Solver: compute ranked solutions
Solver-->>StygianSolutionBoard: return complete solution boards
StygianSolutionBoard-->>Player: render teams, times, videos, and controls
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/solver.ts (1)
1032-1062: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve partial Stygian fallback boards.
When no complete board exists in
allTeams,buildMinMissingStygianSolutionsdiscards every partial result.solveStygianWithFallbackthen returns[], so the UI cannot showneededCharacters. Restore the previous all-team partial fallback or add an explicit replacement behavior and regression test.🤖 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/solver.ts` around lines 1032 - 1062, Update buildMinMissingStygianSolutions and its solveStygianWithFallback flow so partial Stygian results are retained when no complete board exists, rather than discarding every solution with nonempty unfilled. Preserve complete-result prioritization, restore the prior all-team partial fallback behavior (including neededCharacters), and add a regression test covering the no-complete-board case.
🤖 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/app/stygian-cheap-clears.ts`:
- Around line 31-40: Use the exported StygianClearDifficulty union for the
difficulty parameter in cacheKey and the difficulty option type around the opts
handling at the referenced section, including the
STYGIAN_CHEAP_CLEARS_DIFFICULTY default. Preserve the existing validation flow
while ensuring unsupported difficulty values are rejected at compile time.
- Around line 43-66: Extract the duplicated raceAbort helper into one shared
module, preserving its signal.reason fallback and abort-listener cleanup
behavior. In src/lib/app/stygian-cheap-clears.ts lines 43-66 and
src/lib/app/stygian-clear-videos.ts lines 76-99, remove the local
implementations and import the shared helper instead.
In `@src/lib/app/stygian-clear-videos.ts`:
- Around line 127-165: Update the missing-pair fetch flow to split missing into
chunks of MAX_TEAM_ENEMY_PAIRS before calling the API, so each request stays
within the server limit. In the fetch/cache logic around fetchPromise, process
each chunk independently and register inflight entries and cleanup against that
chunk; preserve per-pair caching and await all chunk promises.
In `@src/lib/character-builds.ts`:
- Around line 581-591: Use CharacterBuildExample.invest as the investment-tier
source of truth in exampleHasHighConfig: require example.invest to equal "high"
and retain the existing validation that high_substat_rolls is a non-empty
object. Ensure mid-investment examples remain on the mid sheet even when
high_substat_rolls is present.
- Around line 640-650: Update exampleRelevantGoodKeys and its callers so tier is
propagated consistently: pass tier from exampleRelevantSheetRows and the page
caller, select baseline keys for "mid", and select high-tier keys only for
"high" when high-roll data exists. Adjust high-tier tests to explicitly pass
"high".
In `@src/lib/definitions.ts`:
- Around line 180-184: Update isStygianClearDifficulty to derive its result from
STYGIAN_CLEAR_DIFFICULTY_OPTIONS instead of repeating the "Fearless" and "Dire"
literals, following the options-driven pattern used by isStygianSolverMode.
Preserve the existing type-guard behavior.
In `@src/lib/ui/components/StygianSolutionBoard.svelte`:
- Around line 667-672: Update the fallback note condition and message in
StygianSolutionBoard so it distinguishes video solver mode from yshelper and
hybrid modes. Keep the existing difficulty-based text only for video mode, and
show non-video fallback text explaining that the roster lacks required
characters; do not reference clearDifficulty for yshelper or hybrid.
In `@src/routes/api/stygian-clear-videos/`+server.ts:
- Around line 25-34: Align the isPlaywrightE2e() short-circuit ordering in the
POST handlers for src/routes/api/stygian-clear-videos/+server.ts lines 25-34 and
src/routes/api/stygian-cheap-clears/+server.ts lines 41-48: place the E2E
empty-payload return consistently before enforceApiRateLimit and
requireJsonObject in both endpoints, so E2E requests bypass validation and rate
limiting uniformly.
In `@src/routes/characters/`[slug]/+page.svelte:
- Around line 1589-1597: Add an explicit accessible name to the alternate-team
button in the example picker using the corresponding example metadata, such as
its team name or label. Update the button containing CharacterIcon so screen
readers can identify which alternate team it selects while preserving the
existing selection behavior.
In `@src/routes/dev/ui/`+page.svelte:
- Around line 1201-1206: Update the two gear indicator spans in the sg-d-gear
block to expose their weapon and artifact-set values through semantic roles and
accessible names, using statGoalArchetype.weapon and statGoalArchetype.set
respectively. Preserve the existing visual classes and title attributes while
ensuring the values are available without pointer hover.
- Around line 1136-1149: Update the STAT_GOAL_ARCHETYPES control group to use a
normal button-group pattern: remove the tablist/tab semantics and replace
aria-selected with aria-pressed on each button while preserving the existing
active-state styling and onclick behavior.
In `@src/routes/teams/`[slug]/+page.svelte:
- Line 310: Update the explanatory text near the weapon alternative rows to
define the +1 tag as one extra swap cost and explicitly state that it is not a
pull recommendation, while preserving the existing baseline and DPS context.
---
Outside diff comments:
In `@src/lib/solver.ts`:
- Around line 1032-1062: Update buildMinMissingStygianSolutions and its
solveStygianWithFallback flow so partial Stygian results are retained when no
complete board exists, rather than discarding every solution with nonempty
unfilled. Preserve complete-result prioritization, restore the prior all-team
partial fallback behavior (including neededCharacters), and add a regression
test covering the no-complete-board case.
🪄 Autofix
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: ddb2f936-e4f0-49df-9cd5-332137e092ce
⛔ Files ignored due to path filters (1)
patch-notes/2026-08-13-video-clears-and-build-goals.mdis excluded by none and included by none
📒 Files selected for processing (31)
src/lib/app/stygian-cheap-clears.tssrc/lib/app/stygian-clear-videos.tssrc/lib/build-stats.tssrc/lib/character-builds.test.tssrc/lib/character-builds.tssrc/lib/definitions.tssrc/lib/equipment-data.tssrc/lib/server/request-validation.test.tssrc/lib/server/request-validation.tssrc/lib/solver.test.tssrc/lib/solver.tssrc/lib/stores.tssrc/lib/team-cost.test.tssrc/lib/team-cost.tssrc/lib/types/database.types.tssrc/lib/types/investment.tssrc/lib/ui/components/InfoPopover.sveltesrc/lib/ui/components/InvestmentBuildCard.sveltesrc/lib/ui/components/Select.sveltesrc/lib/ui/components/StygianSolutionBoard.sveltesrc/lib/ui/icons/IconFileSearch.sveltesrc/lib/ui/icons/IconPlay.sveltesrc/routes/api/stygian-cheap-clears/+server.tssrc/routes/api/stygian-clear-videos/+server.tssrc/routes/characters/[slug]/+page.sveltesrc/routes/dev/features/+page.sveltesrc/routes/dev/features/+page.tssrc/routes/dev/ui/+page.sveltesrc/routes/teams/[slug]/+page.sveltesrc/routes/teams/configs/[slug]/+page.sveltesrc/routes/tools/stygian/+page.svelte
| function cacheKey( | ||
| characters: string[], | ||
| stygianVersion: number, | ||
| enemyIds: number[], | ||
| difficulty: string, | ||
| maxCost: number, | ||
| ): string { | ||
| const enemies = [...enemyIds].sort((a, b) => a - b).join(","); | ||
| return `${stygianVersion}:${difficulty}:c${maxCost}:${enemies}:${rosterKey(characters)}`; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Type the difficulty option as StygianClearDifficulty.
opts.difficulty and the cacheKey parameter use string. The default value comes from STYGIAN_CHEAP_CLEARS_DIFFICULTY, and the server rejects any other value with a 400 through requireStygianClearDifficulty. Use the exported union so an invalid difficulty fails at compile time instead of at request time.
♻️ Proposed refactor
import type {
CharacterOwned,
StygianCheapClearRow,
StygianCheapClearsPayload,
+ StygianClearDifficulty,
} from "$lib/definitions"; function cacheKey(
characters: string[],
stygianVersion: number,
enemyIds: number[],
- difficulty: string,
+ difficulty: StygianClearDifficulty,
maxCost: number,
): string { export async function ensureCheapClears(opts: {
owned: CharacterOwned[];
stygianVersion: number;
enemyIds: number[];
- difficulty?: string;
+ difficulty?: StygianClearDifficulty;
maxCost?: number;
signal?: AbortSignal;
}): Promise<StygianCheapClearRow[]> {Also applies to: 72-82
🤖 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/app/stygian-cheap-clears.ts` around lines 31 - 40, Use the exported
StygianClearDifficulty union for the difficulty parameter in cacheKey and the
difficulty option type around the opts handling at the referenced section,
including the STYGIAN_CHEAP_CLEARS_DIFFICULTY default. Preserve the existing
validation flow while ensuring unsupported difficulty values are rejected at
compile time.
| await enforceApiRateLimit({ request, getClientAddress }); | ||
| const body = await requireJsonObject(request); | ||
| const pairs = requireTeamEnemyPairs(body.pairs); | ||
|
|
||
| if (isPlaywrightE2e()) { | ||
| const payload: StygianClearVideosPayload = { clears: [] }; | ||
| return json(payload, { | ||
| headers: { "Cache-Control": "no-store", "X-Playwright-E2E": "1" }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Align the Playwright E2E short-circuit position across the two new endpoints. The two sibling endpoints place the isPlaywrightE2e() branch at different points. One returns the empty payload before rate limiting and body validation. The other rate-limits and validates first. E2E runs then exercise different validation paths per endpoint, and the rate-limit budget is consumed inconsistently.
src/routes/api/stygian-clear-videos/+server.ts#L25-L34: move theisPlaywrightE2e()branch aboveenforceApiRateLimitandrequireJsonObject, or keep it here and document why validation must run.src/routes/api/stygian-cheap-clears/+server.ts#L41-L48: keep one ordering and apply the same ordering in both endpoints.
📍 Affects 2 files
src/routes/api/stygian-clear-videos/+server.ts#L25-L34(this comment)src/routes/api/stygian-cheap-clears/+server.ts#L41-L48
🤖 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/api/stygian-clear-videos/`+server.ts around lines 25 - 34, Align
the isPlaywrightE2e() short-circuit ordering in the POST handlers for
src/routes/api/stygian-clear-videos/+server.ts lines 25-34 and
src/routes/api/stygian-cheap-clears/+server.ts lines 41-48: place the E2E
empty-payload return consistently before enforceApiRateLimit and
requireJsonObject in both endpoints, so E2E requests bypass validation and rate
limiting uniformly.
| ownedVariantsList, | ||
| "Owned weapon options", | ||
| "Already-owned 5★ weapon alternatives compared to this team baseline. The +1 tag is the extra cost for that swap — not a pull recommendation. Highlighted rows are within 2.5% DPS of the best option in this list.", | ||
| "Already-owned 5★ weapon alternatives compared to this team baseline. Highlighted rows are within 2.5% DPS of the best option in this list.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the meaning of the +1 row tag.
The +1 tag is still rendered for owned weapon alternatives, but this text no longer defines it. Users can interpret it as a refinement or pull recommendation. State that +1 means one extra swap cost and is not a pull recommendation.
Proposed wording
- "Already-owned 5★ weapon alternatives compared to this team baseline. Highlighted rows are within 2.5% DPS of the best option in this list.",
+ "Already-owned 5★ weapon alternatives compared to this team baseline. The +1 tag means one extra swap cost, not a pull recommendation. Highlighted rows are within 2.5% DPS of the best option in this list.",📝 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.
| "Already-owned 5★ weapon alternatives compared to this team baseline. Highlighted rows are within 2.5% DPS of the best option in this list.", | |
| "Already-owned 5★ weapon alternatives compared to this team baseline. The +1 tag means one extra swap cost, not a pull recommendation. Highlighted rows are within 2.5% DPS of the best option in this list.", |
🤖 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 310, Update the explanatory
text near the weapon alternative rows to define the +1 tag as one extra swap
cost and explicitly state that it is not a pull recommendation, while preserving
the existing baseline and DPS context.
1 participant
Summary by CodeRabbit
New Features
Improvements