Merged
Conversation
WIP: Release
Zod upgrade release
fix: cache
Migrated events to openapi + autogenerated api client
Enhanced rukia theme and overall looks
Permissions fix
refactor(game-client): simplify chat and guild helpers
Notifications rework
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughAdds normalization and versioned cache keys for characters and guild permissions; introduces user-preference guild ordering and improved wheel handling; extends window components with auto-up-to-max height controls and related store/migration changes; refactors notification expiration scheduling to timeout-based logic and removes per-notification animation state. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lootlog-docs | 0f46e63 | Commit Preview URL Branch Preview URL |
Apr 17 2026, 06:21 PM |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f46e63fe7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <ScrollArea | ||
| className={cn("ll:w-full", className)} | ||
| ref={scrollContainerRef} | ||
| onWheel={handleWheel} |
There was a problem hiding this comment.
onWheel is now wired through React (<ScrollArea onWheel={handleWheel} />), but React 19 registers wheel handlers as passive, so event.preventDefault() in handleWheel is ignored. In scroll layout this means mouse-wheel input will also trigger native vertical page/container scrolling while you manually mutate scrollLeft, which regresses the previous behavior that used a native { passive: false } listener to prevent vertical scrolling during horizontal guild switching.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/game-client/src/hooks/api/use-character-list.tsx (1)
18-28: Redundantselect: normalizeCharacterList.
fetchCharacterListalready returnsMargonemCharacter[]that has been normalized (and additionally filtered byworldand sorted bylvl). Re-runningnormalizeCharacterListinselectis a no-op for correctly-shaped data and only matters if stale persisted query data from a prior version could be hydrated here — but the query key bump tocharacters-v2already invalidates those. You can dropselectunless you're intentionally guarding against a persistence layer (e.g.,persistQueryClient) that may still rehydrate legacy shapes under the new key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/hooks/api/use-character-list.tsx` around lines 18 - 28, The useQuery call is redundantly applying select: normalizeCharacterList even though fetchCharacterList already returns normalized MargonemCharacter[]; remove the select property from the useQuery options in use-character-list (the query created via useQuery with getCharacterListQueryKey) so the queryFn result is returned directly, unless you intentionally need to guard against legacy persisted shapes (persistQueryClient), in which case keep the select. Locate the useQuery invocation that calls fetchCharacterList and drop the select: normalizeCharacterList option.apps/game-client/src/hooks/api/use-guild-permissions.test.ts (1)
28-34: Optional: add a test for mixed/invalid array entries.Current cases cover "valid array" and "non-array". The string-filter branch inside
normalizeGuildPermissionsisn't directly exercised — a test likenormalizeGuildPermissions([Permission.LOOTLOG_MANAGE, 42, null, undefined])asserting it returns only[Permission.LOOTLOG_MANAGE]would lock in the filtering behavior you're relying on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/hooks/api/use-guild-permissions.test.ts` around lines 28 - 34, Add a unit test in use-guild-permissions.test.ts that calls normalizeGuildPermissions with a mixed/invalid array (e.g., [Permission.LOOTLOG_MANAGE, 42, null, undefined]) and asserts the result only contains the valid Permission (e.g., [Permission.LOOTLOG_MANAGE]); place it alongside the existing tests and name it something like "filters out non-string/non-permission entries from arrays" to ensure the string-filter branch in normalizeGuildPermissions is exercised.apps/game-client/src/components/guild-switcher.tsx (1)
66-98: Minor:orderGuildsis computed twice per render.
orderedGuilds(line 67) andorderedGuildsForSelection(line 72) are the same derivation. You can reuse the outer value and list it as a dependency; React Compiler will keep the reference stable across renders when inputs don't change, per the repo's no-useMemoguideline.♻️ Proposed simplification
useEffect(() => { - const orderedGuildsForSelection = orderGuilds(guilds, guildsOrder); - - if (!isFetched || orderedGuildsForSelection.length === 0) return; + if (!isFetched || orderedGuilds.length === 0) return; if (multiple) return; const currentValue = value !== undefined ? value : guildId; if (allowAll && currentValue === "all") return; - const exists = orderedGuildsForSelection.some( - (guild) => guild.id === currentValue, - ); + const exists = orderedGuilds.some((guild) => guild.id === currentValue); if (exists) return; if (onChange) { - onChange(orderedGuildsForSelection[0].id); + onChange(orderedGuilds[0].id); } else { - setGuildId(characterId, orderedGuildsForSelection[0].id); + setGuildId(characterId, orderedGuilds[0].id); } }, [ isFetched, - guilds, - guildsOrder, + orderedGuilds, guildId, value, allowAll, multiple, onChange, characterId, setGuildId, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/guild-switcher.tsx` around lines 66 - 98, The effect recomputes orderGuilds into orderedGuildsForSelection even though orderedGuilds (from orderGuilds(guilds, guildsOrder)) is already available; inside the useEffect replace orderedGuildsForSelection with the outer orderedGuilds, remove the redundant call to orderGuilds, and update the dependency array to include orderedGuilds (and remove guilds/guildsOrder if you prefer relying on orderedGuilds) so the reference stays stable and the duplicate work is eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/game-client/src/api/characters.api.ts`:
- Around line 30-36: The type guard used in the characters normalization only
checks character.id, so legacy/partial entries can pass and later cause NaN or
render crashes; update the predicate in the filter that returns "character is
MargonemCharacter" to validate the other required fields used downstream—at
minimum check typeof character.lvl === "number" and typeof character.world ===
"string", and also ensure typeof character.nick === "string" and typeof
character.icon === "string" (and prof if code depends on it) so only
fully-formed MargonemCharacter objects pass.
In `@apps/game-client/src/components/guild-switcher.test.tsx`:
- Around line 80-97: The test currently uses fireEvent.wheel which only
exercises React's synthetic onWheel path and ignores passive-listener behavior;
update the "scrolls horizontally when the user uses the mouse wheel" test in
GuildSwitcher to (1) stop relying on fireEvent.wheel for validating
prevention-based hijack and instead call the actual wheel handler or exported
helper directly (reference GuildSwitcher and the internal handle/ onWheel
function or exported handler) to assert preventDefault logic and scrollLeft
manipulation, and (2) confirm and use the exact element that scrollContainerRef
is forwarded to by the project's ScrollArea wrapper (ensure the test queries the
same node that scrollContainerRef.current points to rather than assuming the
Radix viewport) so the assertion on scrollLeft targets the real scrolling
element.
In `@apps/game-client/src/components/guild-switcher.tsx`:
- Around line 120-132: The onWheel handler (handleWheel) is currently
ineffective because React attaches wheel listeners as passive so
event.preventDefault() is ignored; change to attaching a non-passive listener in
a useEffect: inside the component useEffect add a 'wheel' event listener on the
ScrollArea DOM node (use scrollContainerRef.current or the element referenced by
ScrollArea) with the same logic as handleWheel but added via
element.addEventListener('wheel', handler, { passive: false }), and remove it in
the cleanup with element.removeEventListener; after adding this effect, remove
the onWheel={handleWheel} prop from the <ScrollArea> so you only rely on the
non-passive listener.
In `@apps/game-client/src/hooks/api/use-guild-permissions.ts`:
- Around line 9-22: The web app's query key is stale: update the permissions
query key definition (the permissions function in apps/web/src/lib/query-keys.ts
that currently returns ["guild-permissions", guildId]) to use the v2 key used by
game-client by returning ["guild-permissions-v2", guildId] as const so both apps
share the same cache key/versioning.
---
Nitpick comments:
In `@apps/game-client/src/components/guild-switcher.tsx`:
- Around line 66-98: The effect recomputes orderGuilds into
orderedGuildsForSelection even though orderedGuilds (from orderGuilds(guilds,
guildsOrder)) is already available; inside the useEffect replace
orderedGuildsForSelection with the outer orderedGuilds, remove the redundant
call to orderGuilds, and update the dependency array to include orderedGuilds
(and remove guilds/guildsOrder if you prefer relying on orderedGuilds) so the
reference stays stable and the duplicate work is eliminated.
In `@apps/game-client/src/hooks/api/use-character-list.tsx`:
- Around line 18-28: The useQuery call is redundantly applying select:
normalizeCharacterList even though fetchCharacterList already returns normalized
MargonemCharacter[]; remove the select property from the useQuery options in
use-character-list (the query created via useQuery with
getCharacterListQueryKey) so the queryFn result is returned directly, unless you
intentionally need to guard against legacy persisted shapes
(persistQueryClient), in which case keep the select. Locate the useQuery
invocation that calls fetchCharacterList and drop the select:
normalizeCharacterList option.
In `@apps/game-client/src/hooks/api/use-guild-permissions.test.ts`:
- Around line 28-34: Add a unit test in use-guild-permissions.test.ts that calls
normalizeGuildPermissions with a mixed/invalid array (e.g.,
[Permission.LOOTLOG_MANAGE, 42, null, undefined]) and asserts the result only
contains the valid Permission (e.g., [Permission.LOOTLOG_MANAGE]); place it
alongside the existing tests and name it something like "filters out
non-string/non-permission entries from arrays" to ensure the string-filter
branch in normalizeGuildPermissions is exercised.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3151c303-381d-4694-982a-9406088fff85
📒 Files selected for processing (9)
apps/game-client/src/api/characters.api.tsapps/game-client/src/components/delete-timer-popover.tsxapps/game-client/src/components/guild-switcher.test.tsxapps/game-client/src/components/guild-switcher.tsxapps/game-client/src/features/timers/components/single-timer.tsxapps/game-client/src/hooks/api/use-character-list.test.tsapps/game-client/src/hooks/api/use-character-list.tsxapps/game-client/src/hooks/api/use-guild-permissions.test.tsapps/game-client/src/hooks/api/use-guild-permissions.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
apps/game-client/src/components/window-resize-handle.test.tsx (2)
59-91: Missing coverage for the both-axes-enabled (default) path.The two tests cover only the disabled-axis branches; the
"se-resize"cursor and combined width+height update path (the most common case in production) is unasserted. Consider adding a third case with both flagstrue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/window-resize-handle.test.tsx` around lines 59 - 91, Add a third test that covers the default/both-axes-enabled path: use renderResizeHandle without overriding allowVerticalResize or allowHorizontalResize (or explicitly set both true) and assert the cursor is "se-resize", then simulate mouseDown/mouseMove/mouseUp (same coordinates as the other tests) and assert handleResize receives both updated width and height (combined change). Locate the test setup using renderResizeHandle, resizeHandle, and handleResize to add this new case.
43-50:offsetWidth/offsetHeightare defined afterrender, but the handle reads them onmouseDown.This works today because the override happens before
fireEvent.mouseDown, but it's fragile — any future change that reads the offsets during render (e.g. measuring in auseLayoutEffect) would flake. Safer to mockHTMLDivElement.prototype.offsetWidth/offsetHeightbeforerender, or to set the properties synchronously via arefcallback. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/window-resize-handle.test.tsx` around lines 43 - 50, The test currently defines offsetWidth/offsetHeight on windowRoot after calling render which is brittle because the component may read offsets during render/useLayoutEffect; move the mock so offsets are defined before render by setting HTMLDivElement.prototype.offsetWidth and HTMLDivElement.prototype.offsetHeight (or alternately set them via the component ref callback synchronously) so that the value is available when the component mounts and when fireEvent.mouseDown runs; update references in the test around render, windowRoot, and the mouseDown event to use the prototype mocks (or ref-based setup) to avoid flakiness.apps/game-client/src/store/notifications.store.test.ts (1)
47-100: Consider also assertinglatestNotificationAnimationCycleis untouched by removal.Nit: the removal test resets the store but doesn't assert that
removeNotificationByNpcIdleaveslatestNotificationAnimationCyclealone (since the cycle is a push-side counter). Cheap extra assertion that locks in intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/store/notifications.store.test.ts` around lines 47 - 100, The test should also assert that removeNotificationByNpcId does not modify the push-side counter latestNotificationAnimationCycle: initialize latestNotificationAnimationCycle in the test state (via useNotificationsStore.setState) to a known value, call useNotificationsStore.getState().removeNotificationByNpcId(500, "pandora"), then add an expectation that useNotificationsStore.getState().latestNotificationAnimationCycle still equals that original value; reference the store accessor useNotificationsStore and the method removeNotificationByNpcId and the property latestNotificationAnimationCycle when adding the assertion.apps/game-client/src/store/windows.store.test.ts (1)
43-79: Test coverage gap: no assertion thatmaxContentHeightis carried through when present.The new test only covers the "legacy limit removed, no existing
maxContentHeight" path. Consider adding a case where the persisted object already has a numericmaxContentHeightand asserting it survives the v6 migration — that's the other branch of thetypeof === "number" ? … : undefinedcheck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/store/windows.store.test.ts` around lines 43 - 79, The test misses the branch where an existing numeric maxContentHeight should be preserved by migrateWindowsState; add a new test (or extend the current one) that passes notifications and "npc-detector" objects with a numeric maxContentHeight (e.g. 120) plus an autoHeightVisibleItemsLimit, call migrateWindowsState, then assert that migrated.notifications.maxContentHeight and migrated["npc-detector"].maxContentHeight equal the original numbers and that autoHeightVisibleItemsLimit has been removed (i.e. not in the resulting records); reference migrateWindowsState and the notifications / "npc-detector" keys to locate where to change tests.apps/game-client/src/components/ui/button.tsx (1)
23-51: Optional: consider ref-as-prop instead offorwardRefon React 19.React 19 supports
refas a regular prop on function components, makingforwardRef(and the explicitdisplayName) unnecessary for new code. Not a bug — leaving it as-is is fine — but a simpler form is available now:Proposed refactor
-import { - forwardRef, - type ComponentProps, - type ForwardedRef, - type MouseEvent, -} from "react"; +import type { ComponentProps, MouseEvent, Ref } from "react"; @@ -type ButtonProps = ComponentProps<"button"> & { - variant?: ButtonVariant; -}; +type ButtonProps = ComponentProps<"button"> & { + variant?: ButtonVariant; + ref?: Ref<HTMLButtonElement>; +}; @@ -export const Button = forwardRef<HTMLButtonElement, ButtonProps>( - function Button( - { className, children, variant = "default", onMouseDown, ...props }, - ref: ForwardedRef<HTMLButtonElement>, - ) { +export const Button = ({ + className, + children, + variant = "default", + onMouseDown, + ref, + ...props +}: ButtonProps) => { const handleMouseDown = (event: MouseEvent<HTMLButtonElement>) => { event.stopPropagation(); onMouseDown?.(event); }; @@ - }, -); - -Button.displayName = "Button"; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/ui/button.tsx` around lines 23 - 51, The component currently uses forwardRef and ForwardedRef; to adopt React 19's simpler ref-as-prop pattern remove forwardRef usage and the ForwardedRef import, update the Button component signature to accept a ref prop (e.g., ref?: Ref<HTMLButtonElement>) inside ButtonProps, pass that ref directly to the <button>, and drop Button.displayName; keep existing logic (handleMouseDown, variant handling, className merging) and ensure ButtonProps typing is updated to include the optional ref to avoid type errors.apps/game-client/src/components/draggable-window.tsx (2)
253-270:useCallbackusage conflicts with the "React Compiler handles memoization" guideline.New code adds
useCallbackforgetResolvedMaxContentHeight, and updateshandleResize/handleResizeStart/handleResizeEnd. Per the project guideline,useMemo/useCallback/memoshould not be used — the React Compiler handles memoization. The rest of the file already usesuseCallback, so this is a pre-existing pattern, but new additions are worth reconsidering while you're touching this code.As per coding guidelines: "React Compiler handles memoization — do not use
memo,useMemo, oruseCallback."Also applies to: 311-354, 356-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/draggable-window.tsx` around lines 253 - 270, Remove the unnecessary useCallback wrapper around getResolvedMaxContentHeight to comply with the "React Compiler handles memoization" guideline and likewise remove useCallback wrappers from handleResize, handleResizeStart, and handleResizeEnd; convert these into plain functions or stable function declarations (keeping the same logic and references to windowBodyRef, contentRef, titleBarRef, localSize.height, and maxContentHeight) so behavior is unchanged but no useCallback/useMemo/memo are used.
573-598: DOM reads during render can force layout thrash while armed.
contentRef.current?.clientHeightandgetMeasuredContentHeight(contentRef.current)are read synchronously during render on every pass (line 575, 577). While the user is dragging with max-height adjustment armed,previewMaxContentHeightstate updates on everymousemove, causing these reads each frame — each one forces a layout flush. Consider cachingmeasuredContentHeightin a ref that's updated from the existingResizeObserver/MutationObserverpath, or reading it only whenisAdjustingMaxHeightis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/draggable-window.tsx` around lines 573 - 598, The render performs synchronous DOM reads (contentRef.current?.clientHeight and getMeasuredContentHeight(contentRef.current)) causing layout thrash during isAdjustingMaxHeight-driven mousemoves; stop reading DOM each render by caching the measured content height in a ref (e.g., measuredContentHeightRef) and update that ref only from the existing ResizeObserver/MutationObserver handlers (or only compute once when isAdjustingMaxHeight becomes true), then use measuredContentHeightRef.current (and avoid calling getMeasuredContentHeight during render) when computing previewShadeOffset/previewBoundaryOffset; ensure you still use previewMaxContentHeight and contentRef identifiers as before and fall back to 0 when the ref is unset.apps/game-client/src/components/draggable-window.test.tsx (1)
7-50: Module-level observer registries: watch for cross-test leakage.
resizeObserverCallbacks,resizeObserverObservedElements, andmutationObserverCallbacksare module-scoped arrays.beforeEachresets them (good), but if any test's component is still mounted when the next test starts (e.g., render without unmount), its observers will also fire whentriggerResizeObservers/triggerMutationObserversare next called. Considercleanup()from@testing-library/reactinafterEach, or asserting the registries are empty after each test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/draggable-window.test.tsx` around lines 7 - 50, Module-level observer registries (resizeObserverCallbacks, resizeObserverObservedElements, mutationObserverCallbacks) can leak between tests if components remain mounted; ensure tests unmount and registries are cleared by calling cleanup() from `@testing-library/react` in an afterEach and then resetting/clearing those arrays (or asserting they are empty) so triggerResizeObservers and triggerMutationObservers only act on currently mounted components. Locate the ResizeObserverMock/MutationObserverMock and the trigger* functions and add an afterEach that calls cleanup(), clears the three arrays, and optionally asserts their lengths are zero to prevent cross-test observer invocation.apps/game-client/src/features/notifications/hooks/use-visible-notifications.ts (1)
13-16: Dead optiontickMscan be removed.
tickMsis no longer used after the switch from polling tosetTimeout. The parameter is prefixed with_tickMsas unused (compliant with guidelines), but the interface fieldtickMs?: numberand the default= 1000can likely be dropped entirely unless kept intentionally for backwards compatibility.Also applies to: 78-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/notifications/hooks/use-visible-notifications.ts` around lines 13 - 16, The interface field tickMs and the unused parameter _tickMs are dead and should be removed: update UseVisibleNotificationsOptions to drop tickMs?: number, remove the _tickMs parameter and its default = 1000 from the useVisibleNotifications hook signature, and delete any internal references/comments about tickMs (also remove the unused default value near the cleanup logic). Keep autoCleanup and ensure the hook compiles/tests pass after removing these unused option bits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/game-client/src/components/draggable-window.tsx`:
- Around line 247-251: The nested ternary computing effectiveHeight (using
isAutoHeightMode, isAdjustingMaxHeight, previewWindowHeight, autoHeight, and
localSize.height) should be replaced with a clear if/else or a small helper
function; locate the effectiveHeight assignment and implement either a helper
like computeEffectiveHeight(isAutoHeightMode, isAdjustingMaxHeight,
previewWindowHeight, autoHeight, localSize) that returns the correct height, or
rewrite inline as if (isAutoHeightMode) { if (isAdjustingMaxHeight &&
previewWindowHeight !== null) return previewWindowHeight; return autoHeight; }
else { return localSize.height; } so the logic is explicit and no chained
ternary remains.
In `@apps/game-client/src/components/window-max-height-action.tsx`:
- Line 40: The visible "{currentMaxHeight}px" string is hardcoded and must be
produced via i18n; replace the literal with a translated string that accepts the
numeric value and unit (e.g., reuse or add a key alongside "maxHeightAria") and
use the app's i18n API to format the number and include the "px" unit from the
locale-aware translation. Locate the JSX that renders {currentMaxHeight}px
(reference currentMaxHeight) in window-max-height-action.tsx and change it to
call the translation helper (e.g., t or formatMessage) with currentMaxHeight as
a variable so locales control number formatting and wording.
In `@apps/game-client/src/components/window-resize-handle.tsx`:
- Around line 59-64: The nested ternary used to compute cursor (using
allowHorizontalResize and allowVerticalResize) reduces readability; replace it
with a small helper or an if/else-if block in the window-resize-handle component
that returns or sets cursor explicitly: if both allowHorizontalResize and
allowVerticalResize set "se-resize", else if allowHorizontalResize set
"ew-resize", else set "ns-resize". Update any references to the cursor variable
to use the helper function or the refactored assignment to keep behavior
identical.
In
`@apps/game-client/src/features/notifications/hooks/use-visible-notifications.ts`:
- Around line 66-75: The check treating missing autoHideState as "paused" causes
early return; update the condition so you first ensure autoHideState exists
before inspecting pausedRemainingMs. In the hook using
notificationAutoHideByListKey and autoHideState, replace the current if
(autoHideState?.pausedRemainingMs !== null) return null; with an existence check
(e.g., if (autoHideState && autoHideState.pausedRemainingMs !== null) return
null;) so that when autoHideState is undefined you fall back to
getExpirationTimeMs(notification, notificationSettings.autoHideTimeout); keep
references to notificationAutoHideByListKey, setNotificationAutoHide,
autoHideState, getExpirationTimeMs and notificationSettings.autoHideTimeout to
locate the change.
In `@apps/game-client/src/features/npc-detector/npc-detector.tsx`:
- Around line 66-72: The JSX attributes are passing a JSX element without braces
causing a syntax error; wrap the element in a braced expression for both
occurrences (the actions prop in the NPC detector and notifications components).
Locate the prop assignment where actions=WindowMaxHeightAction ... and change it
to actions={<WindowMaxHeightAction .../>} (do the same in
apps/game-client/src/features/npc-detector/npc-detector.tsx and
apps/game-client/src/features/notifications/notifications.tsx), keeping the
inner props such as currentMaxHeight={resolvedMaxContentHeight},
isArmed={isMaxHeightAdjustmentArmed}, and onClick={() =>
setIsMaxHeightAdjustmentArmed(currentValue => !currentValue)} unchanged.
In `@apps/game-client/src/store/windows.store.ts`:
- Around line 193-245: Remove the entire version < 5 migration (the block that
sets autoHeightVisibleItemsLimit on state.notifications and
state["npc-detector"]) since it's redundant with the version < 6 work, and then
change the version < 6 migration to bail out when a window entry is missing:
only destructure/transform state.notifications and state["npc-detector"] if they
are truthy (do not spread undefined), e.g. check notifications !== undefined and
npcDetector !== undefined before creating the new objects so you don't replace a
missing window with { maxContentHeight: undefined } — update the code around the
version < 6 block that references state.notifications, notificationsRest,
state["npc-detector"], and npcDetectorRest accordingly.
- Around line 441-447: The setter setMaxContentHeight (accepting key: WindowId,
height: number) currently persists height without validation allowing negative,
NaN or non-finite values; update setMaxContentHeight to validate the incoming
height (e.g., coerce to a finite number and clamp to a sensible minimum like 0
or 1) before updating state, and/or reject non-finite values so
state[key].maxContentHeight is never NaN/Infinity/negative; also reference the
DraggableWindow caller and its use of maxContentHeight/newSize.height and ensure
DraggableWindow applies the same finite-number guard
(Math.fround/Number.isFinite check and Math.max clamp) when passing
newSize.height or reading maxContentHeight.
In `@packages/types/src/common/account-preferences.types.ts`:
- Around line 84-124: The change of ignoreOtherWorlds default to true may
silently alter legacy users' behavior; inspect whether persisted partial
settings exist and then either (A) revert the default in the account-preferences
defaults (the objects with HERO/COLOSSUS/TITAN/message/"party-gathering") to
false so legacy missing fields remain false, or (B) change
normalizeNotificationSettings() in users.service.ts to only apply
fallbackSettings.ignoreOtherWorlds when the stored setting explicitly
hasOwnProperty('ignoreOtherWorlds') === false/true (i.e., treat missing keys as
"preserve existing behaviour") and add a migration or release-note if you
intentionally want to flip behavior; reference normalizeNotificationSettings(),
fallbackSettings, and the ignoreOtherWorlds defaults when making the change.
---
Nitpick comments:
In `@apps/game-client/src/components/draggable-window.test.tsx`:
- Around line 7-50: Module-level observer registries (resizeObserverCallbacks,
resizeObserverObservedElements, mutationObserverCallbacks) can leak between
tests if components remain mounted; ensure tests unmount and registries are
cleared by calling cleanup() from `@testing-library/react` in an afterEach and
then resetting/clearing those arrays (or asserting they are empty) so
triggerResizeObservers and triggerMutationObservers only act on currently
mounted components. Locate the ResizeObserverMock/MutationObserverMock and the
trigger* functions and add an afterEach that calls cleanup(), clears the three
arrays, and optionally asserts their lengths are zero to prevent cross-test
observer invocation.
In `@apps/game-client/src/components/draggable-window.tsx`:
- Around line 253-270: Remove the unnecessary useCallback wrapper around
getResolvedMaxContentHeight to comply with the "React Compiler handles
memoization" guideline and likewise remove useCallback wrappers from
handleResize, handleResizeStart, and handleResizeEnd; convert these into plain
functions or stable function declarations (keeping the same logic and references
to windowBodyRef, contentRef, titleBarRef, localSize.height, and
maxContentHeight) so behavior is unchanged but no useCallback/useMemo/memo are
used.
- Around line 573-598: The render performs synchronous DOM reads
(contentRef.current?.clientHeight and
getMeasuredContentHeight(contentRef.current)) causing layout thrash during
isAdjustingMaxHeight-driven mousemoves; stop reading DOM each render by caching
the measured content height in a ref (e.g., measuredContentHeightRef) and update
that ref only from the existing ResizeObserver/MutationObserver handlers (or
only compute once when isAdjustingMaxHeight becomes true), then use
measuredContentHeightRef.current (and avoid calling getMeasuredContentHeight
during render) when computing previewShadeOffset/previewBoundaryOffset; ensure
you still use previewMaxContentHeight and contentRef identifiers as before and
fall back to 0 when the ref is unset.
In `@apps/game-client/src/components/ui/button.tsx`:
- Around line 23-51: The component currently uses forwardRef and ForwardedRef;
to adopt React 19's simpler ref-as-prop pattern remove forwardRef usage and the
ForwardedRef import, update the Button component signature to accept a ref prop
(e.g., ref?: Ref<HTMLButtonElement>) inside ButtonProps, pass that ref directly
to the <button>, and drop Button.displayName; keep existing logic
(handleMouseDown, variant handling, className merging) and ensure ButtonProps
typing is updated to include the optional ref to avoid type errors.
In `@apps/game-client/src/components/window-resize-handle.test.tsx`:
- Around line 59-91: Add a third test that covers the default/both-axes-enabled
path: use renderResizeHandle without overriding allowVerticalResize or
allowHorizontalResize (or explicitly set both true) and assert the cursor is
"se-resize", then simulate mouseDown/mouseMove/mouseUp (same coordinates as the
other tests) and assert handleResize receives both updated width and height
(combined change). Locate the test setup using renderResizeHandle, resizeHandle,
and handleResize to add this new case.
- Around line 43-50: The test currently defines offsetWidth/offsetHeight on
windowRoot after calling render which is brittle because the component may read
offsets during render/useLayoutEffect; move the mock so offsets are defined
before render by setting HTMLDivElement.prototype.offsetWidth and
HTMLDivElement.prototype.offsetHeight (or alternately set them via the component
ref callback synchronously) so that the value is available when the component
mounts and when fireEvent.mouseDown runs; update references in the test around
render, windowRoot, and the mouseDown event to use the prototype mocks (or
ref-based setup) to avoid flakiness.
In
`@apps/game-client/src/features/notifications/hooks/use-visible-notifications.ts`:
- Around line 13-16: The interface field tickMs and the unused parameter _tickMs
are dead and should be removed: update UseVisibleNotificationsOptions to drop
tickMs?: number, remove the _tickMs parameter and its default = 1000 from the
useVisibleNotifications hook signature, and delete any internal
references/comments about tickMs (also remove the unused default value near the
cleanup logic). Keep autoCleanup and ensure the hook compiles/tests pass after
removing these unused option bits.
In `@apps/game-client/src/store/notifications.store.test.ts`:
- Around line 47-100: The test should also assert that removeNotificationByNpcId
does not modify the push-side counter latestNotificationAnimationCycle:
initialize latestNotificationAnimationCycle in the test state (via
useNotificationsStore.setState) to a known value, call
useNotificationsStore.getState().removeNotificationByNpcId(500, "pandora"), then
add an expectation that
useNotificationsStore.getState().latestNotificationAnimationCycle still equals
that original value; reference the store accessor useNotificationsStore and the
method removeNotificationByNpcId and the property
latestNotificationAnimationCycle when adding the assertion.
In `@apps/game-client/src/store/windows.store.test.ts`:
- Around line 43-79: The test misses the branch where an existing numeric
maxContentHeight should be preserved by migrateWindowsState; add a new test (or
extend the current one) that passes notifications and "npc-detector" objects
with a numeric maxContentHeight (e.g. 120) plus an autoHeightVisibleItemsLimit,
call migrateWindowsState, then assert that
migrated.notifications.maxContentHeight and
migrated["npc-detector"].maxContentHeight equal the original numbers and that
autoHeightVisibleItemsLimit has been removed (i.e. not in the resulting
records); reference migrateWindowsState and the notifications / "npc-detector"
keys to locate where to change tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a26f0c8-a57d-4d35-98fa-d4c7ee95135c
📒 Files selected for processing (20)
apps/game-client/src/components/draggable-window.test.tsxapps/game-client/src/components/draggable-window.tsxapps/game-client/src/components/ui/button.tsxapps/game-client/src/components/window-max-height-action.tsxapps/game-client/src/components/window-resize-handle.test.tsxapps/game-client/src/components/window-resize-handle.tsxapps/game-client/src/features/notifications/components/notifications-list.tsxapps/game-client/src/features/notifications/components/single-notification.tsxapps/game-client/src/features/notifications/hooks/use-visible-notifications.test.tsxapps/game-client/src/features/notifications/hooks/use-visible-notifications.tsapps/game-client/src/features/notifications/notifications.tsxapps/game-client/src/features/npc-detector/components/npcs-list.tsxapps/game-client/src/features/npc-detector/npc-detector.tsxapps/game-client/src/i18n/translations/settings.jsonapps/game-client/src/processors/npcs-delete-processor.test.tsapps/game-client/src/store/notifications.store.test.tsapps/game-client/src/store/notifications.store.tsapps/game-client/src/store/windows.store.test.tsapps/game-client/src/store/windows.store.tspackages/types/src/common/account-preferences.types.ts
💤 Files with no reviewable changes (3)
- apps/game-client/src/processors/npcs-delete-processor.test.ts
- apps/game-client/src/features/notifications/components/single-notification.tsx
- apps/game-client/src/store/notifications.store.ts
✅ Files skipped from review due to trivial changes (2)
- apps/game-client/src/features/npc-detector/components/npcs-list.tsx
- apps/game-client/src/i18n/translations/settings.json
Comment on lines
+66
to
+72
| actions=<WindowMaxHeightAction | ||
| currentMaxHeight={resolvedMaxContentHeight} | ||
| isArmed={isMaxHeightAdjustmentArmed} | ||
| onClick={() => | ||
| setIsMaxHeightAdjustmentArmed((currentValue) => !currentValue) | ||
| } | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is JSX syntax attribute= (without braces) legal in JSX/TSX?
💡 Result:
No, JSX syntax attribute=<Component /> (without braces) is not legal in JSX/TSX.
Citations:
- 1: https://17.reactjs.org/docs/jsx-in-depth.html
- 2: https://eslint.style/rules/jsx-curly-brace-presence
- 3: https://www.typescriptlang.org/docs/handbook/jsx.html
- 4: https://reactjs.org/docs/jsx-in-depth.html
- 5: https://beta.reactjs.org/learn/javascript-in-jsx-with-curly-braces
- 6: https://react.dev/learn/javascript-in-jsx-with-curly-braces
- 7: https://babel.dev/docs/babel-plugin-transform-react-jsx
🏁 Script executed:
fd "notifications.tsx" apps/game-client/src --type fRepository: lootlog/monorepo
Length of output: 193
🏁 Script executed:
sed -n '50,70p' apps/game-client/src/features/notifications/notifications.tsxRepository: lootlog/monorepo
Length of output: 920
JSX syntax error: actions=<WindowMaxHeightAction …/> won't compile in both files.
JSX attribute values must be either a string literal or a braced expression. A JSX element passed as a prop value must be wrapped in { … }. The code contains this syntax error in two locations:
apps/game-client/src/features/npc-detector/npc-detector.tsxlines 66-72apps/game-client/src/features/notifications/notifications.tsxlines 57-63
🛠️ Proposed fix
- actions=<WindowMaxHeightAction
- currentMaxHeight={resolvedMaxContentHeight}
- isArmed={isMaxHeightAdjustmentArmed}
- onClick={() =>
- setIsMaxHeightAdjustmentArmed((currentValue) => !currentValue)
- }
- />
+ actions={
+ <WindowMaxHeightAction
+ currentMaxHeight={resolvedMaxContentHeight}
+ isArmed={isMaxHeightAdjustmentArmed}
+ onClick={() =>
+ setIsMaxHeightAdjustmentArmed((currentValue) => !currentValue)
+ }
+ />
+ }
onClose={handleClose}Apply the same fix to both files.
📝 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
| actions=<WindowMaxHeightAction | |
| currentMaxHeight={resolvedMaxContentHeight} | |
| isArmed={isMaxHeightAdjustmentArmed} | |
| onClick={() => | |
| setIsMaxHeightAdjustmentArmed((currentValue) => !currentValue) | |
| } | |
| /> | |
| actions={ | |
| <WindowMaxHeightAction | |
| currentMaxHeight={resolvedMaxContentHeight} | |
| isArmed={isMaxHeightAdjustmentArmed} | |
| onClick={() => | |
| setIsMaxHeightAdjustmentArmed((currentValue) => !currentValue) | |
| } | |
| /> | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/game-client/src/features/npc-detector/npc-detector.tsx` around lines 66
- 72, The JSX attributes are passing a JSX element without braces causing a
syntax error; wrap the element in a braced expression for both occurrences (the
actions prop in the NPC detector and notifications components). Locate the prop
assignment where actions=WindowMaxHeightAction ... and change it to
actions={<WindowMaxHeightAction .../>} (do the same in
apps/game-client/src/features/npc-detector/npc-detector.tsx and
apps/game-client/src/features/notifications/notifications.tsx), keeping the
inner props such as currentMaxHeight={resolvedMaxContentHeight},
isArmed={isMaxHeightAdjustmentArmed}, and onClick={() =>
setIsMaxHeightAdjustmentArmed(currentValue => !currentValue)} unchanged.
Comment on lines
+519
to
+522
| const resizeObserver = new ResizeObserver(() => { | ||
| updateObservedContentElements(); | ||
| scheduleAutoHeightUpdate(); | ||
| }); |
| @@ -0,0 +1,107 @@ | |||
| import { fireEvent, render, screen, waitFor } from "@testing-library/react"; | |||
Merged
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
New Features
Improvements
Bug Fixes
Tests