Conversation
|
Warning Review limit reached
Next review available in: 34 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 (2)
📝 WalkthroughWalkthroughThe PR adds investment-priority data models and character build displays, filters artifact substat recommendations, extends tooltip detail-sheet behavior, and revises related page and settings presentation text. ChangesCharacter investment priorities
Accessible tooltip detail sheets
Interface copy and layout cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@copilot resolve the merge conflicts in this pull request |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/ui/components/ArtifactTooltip.svelte`:
- Around line 24-33: Derive the combined artifact title once in the component
script, including the piece-count suffix when pieceCount is available, and reuse
that value for both the HoverTooltip label and the visible tip-detail-text
header. Remove the duplicated inline string construction while preserving the
existing formatting and null handling.
In `@src/lib/ui/components/HoverTooltip.svelte`:
- Around line 28-49: Move the updateTriggerDescription calls from
openDetail/closeDetail into showTip/hideTip so aria-describedby is added only
while the hover/focus tooltip is visible and removed when it hides. Keep the
existing updateTriggerDescription helper and detail-sheet behavior unchanged.
- Around line 146-158: Update HoverTooltip’s openDetail handler to avoid
hijacking clicks from interactive or focusable descendants: before calling
preventDefault, stopPropagation, or opening the sheet, detect whether the event
target is an interactive element and return without handling it. Also gate sheet
opening on the tooltip being useful, such as truncated content or an equivalent
existing condition, while preserving normal detail-sheet behavior for eligible
triggers.
In `@src/lib/upgrade-priority.ts`:
- Line 54: Remove the trailing whitespace from the highly_recommended
user-facing label so it renders as “High impact weapon” without altering the
label’s wording or surrounding mappings.
- Around line 1-18: Update the documentation in UpgradeTier and
UpgradeImpactConfig to describe the impact statistic as median percentage impact
rather than mean percentage impact, including the cutoff comments. Keep the tier
names and threshold behavior unchanged.
In `@src/routes/characters/`[slug]/+page.svelte:
- Around line 248-274: Use one statistic consistently for ordering and impact
classification, preferably median: update talentImportanceRows and the signature
weapon rows in src/routes/characters/[slug]/+page.svelte at lines 248-274 and
801-841 to order and classify with median values, and update the module and
UpgradeImpactConfig documentation in src/lib/upgrade-priority.ts at lines 1-18
to describe median-based cutoffs.
In `@src/routes/dev/ui/`+page.svelte:
- Around line 748-752: Remove the inline color style from the tip-detail-text
paragraph inside HoverTooltip so the tooltip’s .hover-tooltip/.tip-sheet color
declaration cascades correctly. Keep the existing markup and class unchanged.
In `@src/routes/teams/`[slug]/+page.svelte:
- Around line 172-174: Update the HoverTooltip usage around NEG_PCT_TIP to
provide a descriptive accessible label instead of the percentage-valued label
variable. Keep the displayed tip content unchanged and use wording that
identifies the tooltip as explaining the negative percentage.
In `@src/routes/teams/`+page.svelte:
- Around line 243-248: Update the page-meta description near the “Team
Simulations” heading so it does not render incomplete “Database of the teams…”
copy while data is null. Condition the entire description on data availability
or show a loading-safe label, while preserving the team count and existing
loaded-state wording when data is present.
🪄 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: 3d5c7313-ebb4-4c0e-b63e-983ac5843377
📒 Files selected for processing (16)
src/lib/build-stats.tssrc/lib/types/investment.tssrc/lib/ui/components/ArtifactTooltip.sveltesrc/lib/ui/components/GameText.sveltesrc/lib/ui/components/HoverTooltip.sveltesrc/lib/ui/components/WeaponTooltip.sveltesrc/lib/upgrade-priority.tssrc/routes/characters/+page.sveltesrc/routes/characters/[slug]/+page.sveltesrc/routes/dev/ui/+page.sveltesrc/routes/settings/+layout.sveltesrc/routes/settings/panels/AccountPanel.sveltesrc/routes/settings/panels/DisplayPanel.sveltesrc/routes/settings/panels/RosterPanel.sveltesrc/routes/teams/+page.sveltesrc/routes/teams/[slug]/+page.svelte
💤 Files with no reviewable changes (1)
- src/routes/settings/+layout.svelte
| function updateTriggerDescription(trigger: HTMLElement | null, add: boolean) { | ||
| if (!trigger) return; | ||
|
|
||
| const describedBy = new Set( | ||
| (trigger.getAttribute("aria-describedby") ?? "") | ||
| .split(/\s+/) | ||
| .filter(Boolean), | ||
| ); | ||
|
|
||
| if (add) { | ||
| describedBy.add(tooltipId); | ||
| } else { | ||
| describedBy.delete(tooltipId); | ||
| } | ||
|
|
||
| if (!describedBy.size) { | ||
| trigger.removeAttribute("aria-describedby"); | ||
| return; | ||
| } | ||
|
|
||
| trigger.setAttribute("aria-describedby", [...describedBy].join(" ")); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
aria-describedby is wired to the wrong state.
updateTriggerDescription is only called from openDetail/closeDetail, so the trigger points at #tooltipId exactly when that element renders nothing (markup is gated on {#if !detailOpen}), and never while the hover/focus tooltip is actually visible. The result: keyboard/SR users focusing the trigger get no description, and while the sheet is open the description resolves to an empty node (the sheet already carries its own aria-label).
Move the add/remove calls into showTip/hideTip instead.
♿ Suggested rewiring
function showTip(trigger: HTMLElement) {
if (detailOpen) return;
open = true;
+ updateTriggerDescription(trigger, true);
requestAnimationFrame(() => {- function hideTip() {
+ function hideTip(trigger?: HTMLElement) {
open = false;
+ if (trigger) updateTriggerDescription(trigger, 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/components/HoverTooltip.svelte` around lines 28 - 49, Move the
updateTriggerDescription calls from openDetail/closeDetail into showTip/hideTip
so aria-describedby is added only while the hover/focus tooltip is visible and
removed when it hides. Keep the existing updateTriggerDescription helper and
detail-sheet behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/dev/ui/+page.svelte (1)
739-743: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the tooltip behavior note conditional on truncation.
HoverTooltipreturns without opening the detail sheet when the content is not truncated, so click/tap does not always open a sheet as this copy claims. Either qualify the text or remove the truncation gate.Suggested copy
- Hover shows a capped tip; click / tap opens a scrollable detail sheet. + Hover shows a capped tip; click / tap opens a scrollable detail sheet + when the content is truncated.🤖 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/dev/ui/`+page.svelte around lines 739 - 743, Update the tooltip behavior note near HoverTooltip to conditionally mention click/tap opening a detail sheet only when the content is truncated, or remove that claim so the copy matches the existing truncation gate. Preserve the note about hover showing a capped tip and the surrounding mockup description.
🤖 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/routes/teams/`[slug]/+page.svelte:
- Around line 163-169: Update the negPctTip snippet so the negative-percentage
tooltip is keyboard-reachable without nesting an interactive trigger inside the
row anchor. Render HoverTooltip outside the link with a dedicated focusable
trigger, or configure it to use the row link as its trigger, while preserving
NEG_PCT_TIP and the existing detail-sheet behavior.
---
Outside diff comments:
In `@src/routes/dev/ui/`+page.svelte:
- Around line 739-743: Update the tooltip behavior note near HoverTooltip to
conditionally mention click/tap opening a detail sheet only when the content is
truncated, or remove that claim so the copy matches the existing truncation
gate. Preserve the note about hover showing a capped tip and the surrounding
mockup description.
🪄 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: 02aa5340-45ca-429c-af16-16690f6969ba
📒 Files selected for processing (7)
src/lib/ui/components/ArtifactTooltip.sveltesrc/lib/ui/components/HoverTooltip.sveltesrc/lib/upgrade-priority.tssrc/routes/characters/[slug]/+page.sveltesrc/routes/dev/ui/+page.sveltesrc/routes/teams/+page.sveltesrc/routes/teams/[slug]/+page.svelte
…le <button> hosting HoverTooltip (keyboard-reachable, no nested interactive in the anchor).
1 participant
Summary by CodeRabbit
New Features
Improvements