Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR adds URL-synchronized filters and navigation history, revised upgrade-impact ladders, reusable popover and display components, fixed-position menus, exact-cost team selection, and an independently revealed pull-suggestion ledger. ChangesUpgrade impact and character builds
URL state and navigation
Shared UI and menus
Teams and pulls
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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: 4
🤖 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/query-state.ts`:
- Around line 64-65: Update the URL construction in the query-state flow around
params.toString() to append url.hash when returning either the filtered or
unfiltered pathname, preserving fragments such as `#results` during replaceState
updates. Add a regression case in the query-state tests covering a URL with an
existing fragment and verifying that it remains in the result.
In `@src/routes/characters/`+page.svelte:
- Around line 23-58: Update the URL-backed filter state initialization and
synchronization in src/routes/characters/+page.svelte (lines 23-58) and
src/routes/teams/+page.svelte (lines 68-99) so navigation changes reconstruct
all local filters from the current user-visible URL instead of retaining cached
$state values; use the route’s reactive URL source such as page.shallow.url, or
window.location.search within the URL-read effect, while preserving the existing
replaceState behavior.
In `@src/routes/pulls/`+page.svelte:
- Line 695: Update the panel heading text in the h2 with class panel-title to
remove the duplicated “used,” while preserving the intended meaning and
surrounding markup.
- Around line 440-460: Update the toggle button in the team-row controls to
remove the transient disabled state driven by isHiding, and guard the onclick
handler so repeated activation is ignored while hiding is in progress without
blurring the focused button. Also remove aria-controls={rowId} unless it can
reference the actual revealed region; retain aria-expanded and the existing
toggle 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: 8a2914e1-5a00-4b76-9f19-fb64f7ebecab
⛔ Files ignored due to path filters (1)
package.jsonis excluded by none and included by none
📒 Files selected for processing (41)
src/app.csssrc/lib/character-builds.test.tssrc/lib/character-builds.tssrc/lib/investment-teams.test.tssrc/lib/investment-teams.tssrc/lib/nav-history.test.tssrc/lib/nav-history.tssrc/lib/pullSuggestions.tssrc/lib/query-state.test.tssrc/lib/query-state.tssrc/lib/ui/NavBar.sveltesrc/lib/ui/components/BackLink.sveltesrc/lib/ui/components/CharacterFilterBar.sveltesrc/lib/ui/components/CharacterTagSearch.sveltesrc/lib/ui/components/CostPopover.sveltesrc/lib/ui/components/HoverTooltip.sveltesrc/lib/ui/components/InfoPopover.sveltesrc/lib/ui/components/PageHeader.sveltesrc/lib/ui/components/Select.sveltesrc/lib/ui/components/StatRow.sveltesrc/lib/ui/components/TeamNumbersNote.sveltesrc/lib/ui/components/UpgradeImpactPopover.sveltesrc/lib/ui/components/UsageIndexPopover.sveltesrc/lib/ui/components/WishSlot.sveltesrc/lib/ui/icons/IconEye.sveltesrc/lib/ui/icons/IconEyeOff.sveltesrc/lib/ui/icons/IconX.sveltesrc/lib/upgrade-priority.test.tssrc/lib/upgrade-priority.tssrc/routes/+layout.sveltesrc/routes/+page.sveltesrc/routes/abyss/+page.sveltesrc/routes/characters/+page.sveltesrc/routes/characters/[slug]/+page.sveltesrc/routes/dev/ui/+page.sveltesrc/routes/pulls/+page.sveltesrc/routes/stygian/+page.sveltesrc/routes/team-configs/[slug]/+page.sveltesrc/routes/teams/+page.sveltesrc/routes/teams/[slug]/+page.sveltetests/pulls.spec.ts
💤 Files with no reviewable changes (1)
- src/lib/ui/components/PageHeader.svelte
Constrained oversized Select menus and added keyboard navigation/focus restoration. Made upgrade strength use absolute magnitude while preserving sign; added negative coverage. Moved constellation/signature impact calculations into tested helpers. Resynced character/team settings-panel visibility from URL navigation. Corrected the dev CRIT Rate icon key to critRate_.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/routes/pulls/+page.svelte (1)
232-240: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a single animation preference across JS and CSS.
animationsEnabledis only set by localStorage/settings, whileprefers-reduced-motionbypasses animations in CSS. In these pull-page timers, JS can keep rows/standouts blocked after CSS animations are already suppressed, or remove them too fast when the store is enabled. Merge the media query and user setting into one helper/store before using it for both timers and CSS.🤖 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/pulls/`+page.svelte around lines 232 - 240, The pull-page timer logic around animationsEnabled must use the same effective animation preference as the CSS, including prefers-reduced-motion and the user setting. Update the shared animation preference helper/store, then use that unified value for wait calculations in the teamHideTimers and related row/standout timers so JavaScript timing stays consistent with CSS animation behavior.src/lib/ui/components/CharacterTagSearch.svelte (2)
60-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNo max-height fallback when the menu fits neither above nor below.
When
above < EDGE,topis clamped into the remaining space butmenu.style.maxHeightis never set, so a long suggestion list simply overflows past the viewport edge with no scroll affordance. On short viewports (e.g. mobile with the on-screen keyboard open) this can make suggestions below the fold unreachable.🐛 Proposed fix
let top = rect.bottom + GAP; + menu.style.maxHeight = ""; if (top + menuRect.height > vh - EDGE) { const above = rect.top - menuRect.height - GAP; if (above >= EDGE) { top = above; } else { const maxTop = Math.max(EDGE, vh - EDGE - menuRect.height); top = Math.max(EDGE, Math.min(top, maxTop)); + menu.style.maxHeight = `${Math.max(0, vh - EDGE - top)}px`; } }Also ensure
.suggestions-dropdownhasoverflow-y: auto;so the constrained height actually scrolls.🤖 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/CharacterTagSearch.svelte` around lines 60 - 83, Update placeMenu so when the menu fits neither above nor below (the above < EDGE fallback), calculate and apply a maxHeight that fits within the available viewport space. Ensure the .suggestions-dropdown styling uses overflow-y: auto so constrained suggestion lists remain scrollable.
94-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDropdown doesn't reposition as the suggestion list changes while typing.
The effect only tracks
showSuggestions(a boolean). SinceshowSuggestionsstaystrueacross most keystrokes even assuggestions.lengthshrinks or grows,placeMenu()is computed once per open and never re-run as the visible list — and therefore the menu's natural height — changes. This can leave the dropdown mispositioned (e.g. still placed above the field with a stale gap, or overlapping) as the user narrows/widens their search.🐛 Proposed fix
$effect(() => { if (!showSuggestions) return; + // Track the suggestion list itself so repositioning also runs when its + // size changes (not just when the dropdown opens/closes). + suggestions.length; void tick().then(() => { placeMenu(); requestAnimationFrame(placeMenu); });🤖 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/CharacterTagSearch.svelte` around lines 94 - 112, Update the reactive effect containing placeMenu so it also depends on the changing suggestions collection or its length, causing repositioning whenever the visible suggestion list changes while showSuggestions remains true. Preserve the existing tick, animation-frame, resize, scroll, and cleanup behavior.
🤖 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/character-builds.ts`:
- Around line 116-156: Extract the duplicated impact mapping from
constellationImpactRows and rankSigWeaponsByGain into a shared attachImpact
helper that accepts a gain row and UpgradeImpactLadder, computes pct via
primaryUpgradePct, classifies it, and returns the enriched row. Update both
functions to reuse the helper while preserving constellation source order and
signature-weapon sorting.
In `@src/lib/ui/components/Select.svelte`:
- Around line 141-164: Scope the window-level onKeydown handler to this Select
by first verifying that document.activeElement is contained by triggerEl or
menuEl, and return without handling the event otherwise. Apply this guard before
processing Escape or navigation keys so unrelated focused elements retain their
normal key behavior.
---
Outside diff comments:
In `@src/lib/ui/components/CharacterTagSearch.svelte`:
- Around line 60-83: Update placeMenu so when the menu fits neither above nor
below (the above < EDGE fallback), calculate and apply a maxHeight that fits
within the available viewport space. Ensure the .suggestions-dropdown styling
uses overflow-y: auto so constrained suggestion lists remain scrollable.
- Around line 94-112: Update the reactive effect containing placeMenu so it also
depends on the changing suggestions collection or its length, causing
repositioning whenever the visible suggestion list changes while showSuggestions
remains true. Preserve the existing tick, animation-frame, resize, scroll, and
cleanup behavior.
In `@src/routes/pulls/`+page.svelte:
- Around line 232-240: The pull-page timer logic around animationsEnabled must
use the same effective animation preference as the CSS, including
prefers-reduced-motion and the user setting. Update the shared animation
preference helper/store, then use that unified value for wait calculations in
the teamHideTimers and related row/standout timers so JavaScript timing stays
consistent with CSS animation 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: 77440e76-baf5-4528-8e35-74817b1e90f8
📒 Files selected for processing (13)
src/lib/character-builds.test.tssrc/lib/character-builds.tssrc/lib/query-state.test.tssrc/lib/query-state.tssrc/lib/ui/components/CharacterTagSearch.sveltesrc/lib/ui/components/Select.sveltesrc/lib/upgrade-priority.test.tssrc/lib/upgrade-priority.tssrc/routes/characters/+page.sveltesrc/routes/characters/[slug]/+page.sveltesrc/routes/dev/ui/+page.sveltesrc/routes/pulls/+page.sveltesrc/routes/teams/+page.svelte
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