Merged
Conversation
fix: notifications spacing
fix: detector rules
|
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 39 minutes and 42 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 (4)
📝 WalkthroughWalkthroughAdds multiple new agent/Claude skill spec files, restructures settings i18n from a monolithic JSON into many modular files, integrates i18n across the game client UI, implements centralized command parsing/state and related UI/components, refactors timer color-name resolution to i18n-backed getters, and adds extensive test coverage across notifications, detector, timers, and settings. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as CommandWindow (Client)
participant Parser as getCommandState
participant Suggestions as CommandSuggestions
participant Party as PartyHandler
participant NotifAPI as Notification API
participant ChatAPI as Chat API
UI->>Parser: onInput(message)
Parser-->>Suggestions: compute commandQuery / open state
Suggestions-->>UI: show suggestion list
UI->>Parser: onSubmit(message)
Parser-->>UI: return CommandState (mode, canSubmit, submissionMessage)
alt mode == "party"
UI->>Party: handlePartyCommand(submissionMessage, selectedGuilds)
Party-->>ChatAPI: send PARTY message
else mode == "notification"
UI->>NotifAPI: createNotification(submissionMessage, recipients)
NotifAPI-->>ChatAPI: may send NOTIFICATION chat message
else mode == "normal"
UI->>ChatAPI: send NORMAL chat message (submissionMessage)
end
ChatAPI-->>UI: ack / close window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
![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: 6c2211aec8
ℹ️ 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".
Comment on lines
+51
to
+53
| const hasContent = isCommand | ||
| ? commandPayload.length > 0 | ||
| : inputValue.trim().length > 0; |
There was a problem hiding this comment.
!grp without requiring a payload
getCommandState currently treats all command-prefixed inputs as requiring non-empty commandPayload, which makes !grp (or !grp ) unsendable because canSubmit becomes false. That regresses party-gathering behavior: usePartyCommand accepts description: string | undefined, and the legacy chat input still supports !grp with an empty description, so users can no longer trigger party search from the new command window unless they add text.
Useful? React with 👍 / 👎.
# Conflicts: # apps/game-client/src/features/settings/components/detector/detector-routing-settings-tab-form.test.tsx # apps/game-client/src/i18n/translations/settings.json
Contributor
Resolved and pushed in commit This merge-conflict resolution had no UI changes (code/test/translation merge only). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/game-client/src/components/world-selector.tsx (1)
60-96:⚠️ Potential issue | 🟠 MajorRemove manual memoization around translated groups.
Line 60 uses
useMemowhich violates the React Compiler memoization rule for TSX files. React Compiler handles this optimization automatically—manual memoization withuseMemo,memo, anduseCallbackis not needed.♻️ Proposed refactor
-import { type FC, useEffect, useMemo } from "react"; +import { type FC, useEffect } from "react";- const worldGroups = useMemo<ComboboxGroup[]>(() => { + const worldGroups: ComboboxGroup[] = (() => { if (!worlds || worlds.length === 0) return []; const recent = recentWorlds @@ return groups; - }, [t, worlds, recentWorlds]); + })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/world-selector.tsx` around lines 60 - 96, Remove the manual useMemo wrapper around the worldGroups computation: replace the const worldGroups = useMemo<ComboboxGroup[]>(() => { ... }, [t, worlds, recentWorlds]) with a plain const worldGroups = (() => { ... })() or inline computation so the same logic that builds recent, recentValues, rest and groups using t, worlds and recentWorlds remains but without useMemo; keep the same variable names (recent, recentValues, rest, groups) and the ComboboxGroup shape so behavior is unchanged.apps/game-client/src/components/ui/autocomplete-suggestions.tsx (1)
2-26:⚠️ Potential issue | 🟡 MinorDefault-param
i18n.t(...)won't react to language changes.Using
i18n.t(...)as a default parameter resolves at call time but the component will not re-render when the active language changes, because it isn't subscribed via theuseTranslationhook. If the user switches language while this component is mounted with nonoResultsMessageprop, the displayed text will remain stale until the component re-renders for an unrelated reason.Prefer resolving the default inside the component body via
useTranslation, which also keeps behavior consistent with the rest of the app (e.g.,window-title-bar.tsx).♻️ Proposed change
-import type React from "react"; -import i18n from "@/i18n/config"; +import type React from "react"; +import { useTranslation } from "react-i18next"; @@ export const AutocompleteSuggestions = <T,>({ items, isOpen, onSelect, selectedIndex, renderItem, keyExtractor, - noResultsMessage = i18n.t("settings.command.suggestions.noResults"), + noResultsMessage, showNoResults = false, className = "", }: AutocompleteSuggestionsProps<T>) => { + const { t } = useTranslation(); + const resolvedNoResultsMessage = + noResultsMessage ?? t("settings.command.suggestions.noResults"); const hasResults = items.length > 0; @@ - <p className="ll:text-xs ll:text-gray-400 ll:text-center ll:px-3 ll:py-2"> - {noResultsMessage} - </p> + <p className="ll:text-xs ll:text-gray-400 ll:text-center ll:px-3 ll:py-2"> + {resolvedNoResultsMessage} + </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/ui/autocomplete-suggestions.tsx` around lines 2 - 26, The default i18n.t(...) call in the AutocompleteSuggestions component signature is evaluated once and won't update on language changes; instead import and call useTranslation() inside the AutocompleteSuggestions function and derive noResultsMessage from t(...) when the prop is undefined (e.g., const { t } = useTranslation(); const resolvedNoResultsMessage = noResultsMessage ?? t("settings.command.suggestions.noResults")), leaving the prop signature and other props (items, isOpen, onSelect, selectedIndex, renderItem, keyExtractor, showNoResults, className) unchanged so the component re-renders with the correct localized text when language changes.apps/game-client/src/features/chat/chat.tsx (1)
155-166:⚠️ Potential issue | 🔴 CriticalFix invalid JSX prop syntax: wrap element expressions in braces.
Line 161 uses invalid TSX syntax (
actions=<ChatWindowActions ... />); JSX elements passed as props must be wrapped in braces:actions={<ChatWindowActions ... />}.This pattern appears in 20+ additional files across the codebase and must be fixed systematically. Files include:
apps/web/src/features/guild/stats/components/loot-top-npcs-chart.tsx(lines 108, 117)apps/web/src/features/guild/stats/components/loot-rarity-chart.tsx(lines 98, 138)apps/web/src/features/guild/stats/components/loot-timeline-chart.tsx(line 120)apps/web/src/features/guild/settings/roles/roles.tsx(line 70)apps/game-client/src/features/command/command.tsx(line 136)apps/game-client/src/features/npc-detector/npc-detector.tsx(line 66)- And others—search for the pattern with:
rg '\b[a-zA-Z_$][\w$-]*=<[A-Z]' --glob='**/*.{tsx,jsx}'Proposed fix for chat.tsx
minHeight={116} minWidth={242} - actions=<ChatWindowActions - chatInputEnabled={isChatInputEnabled} - toggleChatInputEnabled={toggleChatInputEnabled} - filtersVisible={filtersVisible} - toggleFiltersVisible={toggleFiltersVisible} - /> + actions={ + <ChatWindowActions + chatInputEnabled={isChatInputEnabled} + toggleChatInputEnabled={toggleChatInputEnabled} + filtersVisible={filtersVisible} + toggleFiltersVisible={toggleFiltersVisible} + /> + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/chat/chat.tsx` around lines 155 - 166, The JSX prop assignment for the DraggableWindow component is using invalid syntax (actions=<ChatWindowActions ... />); update the prop to pass a JSX element expression by wrapping it in braces: change the actions prop on DraggableWindow to actions={<ChatWindowActions ... />} so the ChatWindowActions element (used here) is treated as a JSX expression. Locate the DraggableWindow usage and similarly fix any other instances that follow the pattern identifier=<Component ... /> (e.g., places referencing ChatWindowActions, DraggableWindow, or other components) by wrapping the element value in braces.apps/game-client/src/features/npc-detector/components/npc-list-item.tsx (1)
420-600:⚠️ Potential issue | 🟠 MajorAdd accessible names and reuse computed action labels.
The notification and gather buttons are icon-only, so tooltip text alone does not give them a reliable accessible name. Compute translated labels once and use them for both
aria-labeland tooltip content; this also removes the nested ternary in Line 595.♿ Proposed fix
const background = getBackgroundColor(key, settingsByNpcType?.highlight); const borderColor = getBorderColor(key, settingsByNpcType?.highlight); + const notificationActionLabel = npc.notificationSent + ? t("settings.detector.runtime.notificationSent") + : t("settings.detector.runtime.sendNotification"); + + let gatherActionLabel = t("settings.detector.runtime.gatherParty"); + if (isGatheringPartyPending) { + gatherActionLabel = t("settings.detector.runtime.gatherPending"); + } else if (hasActivePartyGathering) { + gatherActionLabel = t("settings.detector.runtime.alreadyGathering"); + } return ( @@ <Button variant="ghost" className={`ll:relative ${ACTION_BUTTON_CLASS_NAME}`} disabled={isCreateNotificationPending || npc.notificationSent} onClick={() => handleSendNotification(npc)} + aria-label={notificationActionLabel} > @@ <TooltipContent side="top"> - {npc.notificationSent - ? t("settings.detector.runtime.notificationSent") - : t("settings.detector.runtime.sendNotification")} + {notificationActionLabel} </TooltipContent> @@ <Button variant="ghost" className={ACTION_BUTTON_CLASS_NAME} disabled={isGatheringPartyPending || hasActivePartyGathering} onClick={() => handleGatherParty(npc)} + aria-label={gatherActionLabel} > @@ <TooltipContent side="top"> - {isGatheringPartyPending - ? t("settings.detector.runtime.gatherPending") - : hasActivePartyGathering - ? t("settings.detector.runtime.alreadyGathering") - : t("settings.detector.runtime.gatherParty")} + {gatherActionLabel} </TooltipContent>As per coding guidelines, “Avoid nested (chained) ternary expressions — use early returns or
if/else ifinstead.”🤖 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/components/npc-list-item.tsx` around lines 420 - 600, Compute translated action label strings once (e.g., notificationLabel, notificationSentLabel, gatherLabel, gatherPendingLabel, gatherAlreadyLabel) and reuse them for both the Button aria-label and the TooltipContent instead of relying on tooltip-only accessibility; update the Buttons in the icon-only controls (the Button that calls handleSendNotification and the Button that calls handleGatherParty) to include aria-label={notificationLabel} or aria-label={gatherLabel} and pass the same variables into TooltipContent. Also replace the nested ternary used to choose the gather tooltip text with a small if/else or conditional variable assignment (using hasActivePartyGathering and isGatheringPartyPending) so the tooltip selection logic is clear and reusable.apps/game-client/src/features/timers/components/timers-actions.tsx (1)
34-128:⚠️ Potential issue | 🟠 MajorWrap the icon actions in accessible buttons.
These SVGs handle clicks directly, so they are not keyboard-operable controls and the localized tooltip text is not exposed as an accessible name. Use native buttons with translated
aria-labels, and mark the icons decorative.♿ Proposed pattern
}) => { const { t } = useTranslation(); + const filtersLabel = timerFiltersEnabled + ? t("settings.timers.actions.hideFilters") + : t("settings.timers.actions.showFilters"); return [ <Tooltip key="filters-tooltip"> <TooltipTrigger asChild> - <Filter - key="filters" - className="ll-custom-cursor-pointer ll:mt-0.5 ll:stroke-gray-300 ll:hover:stroke-gray-100 ll:transition-colors" - size="14" + <button + type="button" + aria-label={filtersLabel} + className="ll-custom-cursor-pointer ll:mt-0.5 ll:p-0 ll:bg-transparent ll:border-0" onClick={toggleTimerFiltersEnabled} - /> + > + <Filter + aria-hidden="true" + className="ll:stroke-gray-300 ll:hover:stroke-gray-100 ll:transition-colors" + size="14" + /> + </button> </TooltipTrigger> - <TooltipContent side="top"> - {timerFiltersEnabled - ? t("settings.timers.actions.hideFilters") - : t("settings.timers.actions.showFilters")} - </TooltipContent> + <TooltipContent side="top">{filtersLabel}</TooltipContent> </Tooltip>,Apply the same pattern to the palette, sort, and hidden-timer actions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/timers/components/timers-actions.tsx` around lines 34 - 128, The icon SVGs (Filter, Palette, SortDesc, SortAsc, Eye, EyeOff) must be wrapped in native buttons so they are keyboard-operable and have accessible names: replace the SVGs as direct click targets with <button> elements (used as the child of TooltipTrigger) that call the existing handlers (toggleTimerFiltersEnabled, toggleColorFiltersEnabled, setTimersSortOrder, setShowHiddenTimers) and supply translated aria-labels via t(...) (use the same strings currently in TooltipContent). Keep icons decorative by removing interactive handlers from them and setting aria-hidden/focusable={false} on the SVGs, and move any visual classNames you need to the button so styling remains. Ensure the conditional logic still uses timerFiltersEnabled, colorFiltersEnabled, timersSortOrder, and showHiddenTimers to choose labels and styles.
🟡 Minor comments (13)
.agents/skills/design-taste-frontend/SKILL.md-134-135 (1)
134-135:⚠️ Potential issue | 🟡 MinorNormalize heading spelling and platform naming for clarity.
Use
Navigation & MenusandmacOSto avoid documentation noise and search inconsistency.Suggested fix
-### Navigation & Menüs -* **Mac OS Dock Magnification:** Nav-bar at the edge; icons scale fluidly on hover. +### Navigation & Menus +* **macOS Dock Magnification:** Nav-bar at the edge; icons scale fluidly on hover.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/design-taste-frontend/SKILL.md around lines 134 - 135, Update the heading and platform naming to be consistent: replace the heading "Navigation & Menüs" with "Navigation & Menus" and change "Mac OS" to "macOS" within the paragraph (the string containing "Mac OS Dock Magnification" / "Nav-bar at the edge; icons scale fluidly on hover.") so documentation spelling and platform naming are normalized for search and clarity..agents/skills/full-output-enforcement/SKILL.md-37-39 (1)
37-39:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
Line 37 uses an unlabeled fence, which trips markdownlint
MD040. Use a language tag (e.g.,text) to keep docs lint-clean.Suggested fix
-``` +```text [PAUSED — X of Y complete. Send "continue" to resume from: next section name]</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/full-output-enforcement/SKILL.md around lines 37 - 39, The
fenced code block containing the literal line "[PAUSED — X of Y complete. Send
"continue" to resume from: next section name]" is unlabeled and triggers
markdownlint MD040; update that fence in SKILL.md by changing the openingto a language-tagged fence such astext so the block becomestext ...to
satisfy the linter while preserving the block content.</details> </blockquote></details> <details> <summary>.agents/skills/high-end-visual-design/SKILL.md-1-4 (1)</summary><blockquote> `1-4`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a Skills & Capabilities section to AGENTS.md documenting the new skills.** This PR adds six new skill definitions to `.agents/skills/`, but AGENTS.md has no Skills section to document them. Per project learnings, agent definitions and capabilities should be documented in AGENTS.md for visibility. Please add a section listing all available skills and their purposes (e.g., `high-end-visual-design`, `full-output-enforcement`, `industrial-brutalist-ui`, `design-taste-frontend`, `minimalist-ui`, `stitch-design-taste`) alongside the existing instructions. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/high-end-visual-design/SKILL.md around lines 1 - 4, Add a "Skills & Capabilities" section to AGENTS.md that documents the six new skill definitions added under .agents/skills/: high-end-visual-design, full-output-enforcement, industrial-brutalist-ui, design-taste-frontend, minimalist-ui, and stitch-design-taste; for each skill include a one-line purpose/summary (e.g., "high-end-visual-design — enforces agency-grade fonts, spacing, shadows, card structure and animations") and place this section alongside the existing instructions section so agent definitions and capabilities are visible in the same top-level doc. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/timers/components/timers-empty-state.tsx-15-15 (1)</summary><blockquote> `15-15`: _⚠️ Potential issue_ | _🟡 Minor_ **Localize the default empty state too.** Line 15 still hardcodes visible fallback text. Add a translation key for this branch as well, even if the Polish value remains `----`. <details> <summary>🌐 Proposed fix</summary> ```diff - {areFiltersActive ? t("settings.timers.emptyState.filtered") : "----"} + {areFiltersActive + ? t("settings.timers.emptyState.filtered") + : t("settings.timers.emptyState.default")} ``` </details> As per coding guidelines, `apps/*/src/**/*.{jsx,tsx}`: All static text in frontend apps must use i18n — never hardcode user-facing strings. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/timers/components/timers-empty-state.tsx` at line 15, The default fallback text is hardcoded; update the component to use i18n like the filtered branch by adding a new translation key (e.g. "settings.timers.emptyState.default") and replacing the raw "----" with t("settings.timers.emptyState.default"); modify the translations resource(s) (including Polish) to include this key (Polish value can stay "----") and ensure the component still uses areFiltersActive and t(...) as currently implemented. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/settings/components/timers/timers-settings-colors.tsx-68-68 (1)</summary><blockquote> `68-68`: _⚠️ Potential issue_ | _🟡 Minor_ **Replace `||` with `??` for nullish coalescing on line 68.** Line 68 uses `||` which incorrectly treats empty strings as missing values, replacing intentionally saved empty custom names with the default translated name. Use `??` instead to preserve empty strings while still falling back on `null` or `undefined`. <details> <summary>Proposed fix</summary> ```diff - name: defaultColorNames[colorId] || getDefaultColorName(colorId) || "", + name: defaultColorNames[colorId] ?? getDefaultColorName(colorId) ?? "", ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/settings/components/timers/timers-settings-colors.tsx` at line 68, The current assignment for the name property uses the logical OR which treats empty strings as missing (name: defaultColorNames[colorId] || getDefaultColorName(colorId) || ""), so replace the ORs with nullish coalescing to preserve intentionally empty names: use defaultColorNames[colorId] ?? getDefaultColorName(colorId) ?? "" in the code path that builds the object (referencing defaultColorNames, getDefaultColorName and colorId) to ensure only null/undefined fallbacks occur. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/components/timer-settings-conflict-dialog.tsx-25-28 (1)</summary><blockquote> `25-28`: _⚠️ Potential issue_ | _🟡 Minor_ **Handle timestamp `0` correctly in the missing-date check.** Line 26 uses `if (!date)` which treats the valid timestamp `0` as missing, incorrectly returning the unknown-date fallback instead of formatting it as `1970-01-01T00:00:00.000Z` equivalent. <details> <summary>Fix</summary> ```diff const formatDate = (date: Date | number | undefined) => { - if (!date) return t("settings.timerSettingsConflict.unknownDate"); + if (date === undefined) return t("settings.timerSettingsConflict.unknownDate"); const d = typeof date === "number" ? new Date(date) : date; return d.toLocaleString(); }; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/components/timer-settings-conflict-dialog.tsx` around lines 25 - 28, The formatDate function incorrectly treats timestamp 0 as missing due to the falsy check; change the missing-date check in formatDate to explicitly test for null/undefined (e.g., date == null or date === null || date === undefined) so that numeric 0 is considered valid, then keep the existing typeof date === "number" branch to convert numbers to Date and call toLocaleString; ensure the early return still uses t("settings.timerSettingsConflict.unknownDate"). ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/party-finder/get-create-party-gathering-error-message.ts-27-31 (1)</summary><blockquote> `27-31`: _⚠️ Potential issue_ | _🟡 Minor_ **Use nullish fallback for the response message.** Line 29 uses `||`, so an empty server-provided message is treated as missing. Prefer `??` unless blank messages must be explicitly rejected. <details> <summary>Proposed fix</summary> ```diff if (responseStatus === 400) { return ( - error.response?.data?.message || + error.response?.data?.message ?? i18n.t("settings.partyFinder.errors.invalidData") ); } ``` </details> As per coding guidelines, “Prefer `??` over `||` for nullish coalescing”. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/party-finder/get-create-party-gathering-error-message.ts` around lines 27 - 31, Replace the boolean OR fallback with a nullish coalescing fallback in the return for the responseStatus === 400 branch: when returning error.response?.data?.message || i18n.t("settings.partyFinder.errors.invalidData"), use the nullish coalescing operator (??) so an empty string from error.response?.data?.message is preserved rather than treated as missing; update the expression where responseStatus, error.response?.data?.message, and i18n.t(...) are used. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/i18n/translations/settings/party-finder.json-2-2 (1)</summary><blockquote> `2-2`: _⚠️ Potential issue_ | _🟡 Minor_ **Localize the window title in the Polish bundle.** This is the only top-level label left in English while the rest of the resource is Polish. <details> <summary>🌐 Proposed copy update</summary> ```diff - "windowTitle": "Party finder", + "windowTitle": "Wyszukiwarka grupy", ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/i18n/translations/settings/party-finder.json` at line 2, The Polish translation bundle still has the top-level label "windowTitle" in English; update the value of the "windowTitle" key in the party-finder.json Polish resource to the proper Polish string (e.g., "Wyszukiwarka drużyn" or other agreed translation) so the entire resource is localized; locate the "windowTitle" entry in apps/game-client/src/i18n/translations/settings/party-finder.json and replace the English text with the Polish copy. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/i18n/translations/settings/detector.json-16-16 (1)</summary><blockquote> `16-16`: _⚠️ Potential issue_ | _🟡 Minor_ **Keep the empty-state copy fully localized.** `Detector` stands out as English in the Polish translation bundle. <details> <summary>🌐 Proposed copy update</summary> ```diff - "emptyState": "Brak reguł routingu. Detector będzie działał lokalnie bez wysyłki na Discord.", + "emptyState": "Brak reguł routingu. Wykrywacz będzie działał lokalnie bez wysyłki na Discord.", ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/i18n/translations/settings/detector.json` at line 16, The Polish translation for the empty state contains an English word "Detector"; update the value for the "emptyState" key to use fully localized Polish (e.g., replace "Detector" with "Detektor" and keep the rest of the sentence intact) so the string reads like "Brak reguł routingu. Detektor będzie działał lokalnie bez wysyłki na Discord." Ensure you edit the "emptyState" entry in the detector translation JSON (key "emptyState") to perform this change. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/settings/components/notifications/notifications-settings-tab.test.tsx-73-93 (1)</summary><blockquote> `73-93`: _⚠️ Potential issue_ | _🟡 Minor_ **Test the `loading` delayed state too.** This test name covers “loading and saving”, but it only exercises `saving`, so a regression in the `loading` branch would still pass. <details> <summary>🧪 Proposed fix</summary> ```diff - it("delays visible sync status updates for loading and saving states", () => { + it.each(["loading", "saving"] as const)( + "delays visible sync status updates for %s state", + (status) => { vi.useFakeTimers(); const { rerender } = render(<NotificationsSettingsTab />); expect(screen.getByTestId("sync-status")).toHaveTextContent("idle"); mockUseGameAccountPreferencesSyncStatus.mockReturnValue({ - status: "saving", + status, error: null, }); rerender(<NotificationsSettingsTab />); expect(screen.getByTestId("sync-status")).toHaveTextContent("idle"); @@ - expect(screen.getByTestId("sync-status")).toHaveTextContent("saving"); - }); + expect(screen.getByTestId("sync-status")).toHaveTextContent(status); + }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/settings/components/notifications/notifications-settings-tab.test.tsx` around lines 73 - 93, The test in NotificationsSettingsTab.test should also exercise the "loading" branch: after initial render and idle assertion, set mockUseGameAccountPreferencesSyncStatus to return { status: "loading", error: null }, rerender NotificationsSettingsTab, assert the visible sync-status remains "idle", advance fake timers by the same delay used for debouncing, then assert the sync-status shows "loading"; then proceed to change the mock to { status: "saving", error: null }, rerender, repeat the delay/advance and assert "saving" to cover both branches (use the existing vi.useFakeTimers/act/vi.advanceTimersByTime and the same test utilities). ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/party-finder/components/create-party-gathering-form.tsx-21-39 (1)</summary><blockquote> `21-39`: _⚠️ Potential issue_ | _🟡 Minor_ **Localize all user-facing Zod error messages in this schema.** The cross-field level comparison is translated, but `description.max(200)` and the numeric validators for `minLvl`/`maxLvl` (.min(1), .max(500)) still emit default Zod error messages. These are rendered directly to users at lines 176–180 and 209–213, violating the i18n guideline for all static text in frontend apps. Pass the `t` function to `getFormSchema` and add custom messages to each validator: <details> <summary>Suggested fix</summary> ```diff +import type { TFunction } from "i18next"; import { useTranslation } from "react-i18next"; -const getFormSchema = (levelErrorMessage: string) => +const getFormSchema = (t: TFunction) => z .object({ - description: z.string().max(200).optional(), - minLvl: z.coerce.number().min(1).max(500).optional().or(z.literal("")), - maxLvl: z.coerce.number().min(1).max(500).optional().or(z.literal("")), + description: z + .string() + .max( + 200, + t("settings.partyFinder.createForm.descriptionTooLong", { + count: 200, + }), + ) + .optional(), + minLvl: z.coerce + .number() + .min(1, t("settings.partyFinder.createForm.levelMin", { min: 1 })) + .max(500, t("settings.partyFinder.createForm.levelMax", { max: 500 })) + .optional() + .or(z.literal("")), + maxLvl: z.coerce + .number() + .min(1, t("settings.partyFinder.createForm.levelMin", { min: 1 })) + .max(500, t("settings.partyFinder.createForm.levelMax", { max: 500 })) + .optional() + .or(z.literal("")), }) .refine( (data) => { @@ -35,7 +52,7 @@ }, { - message: levelErrorMessage, + message: t("settings.partyFinder.createForm.minMaxLevelError"), path: ["minLvl"], }, ); @@ -61,7 +78,7 @@ reset, } = useForm<FormData>({ resolver: zodResolver( - getFormSchema(t("settings.partyFinder.createForm.minMaxLevelError")), + getFormSchema(t), ) as never, ``` Add these translation keys to your `createForm` message bundle. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/party-finder/components/create-party-gathering-form.tsx` around lines 21 - 39, getFormSchema currently uses default Zod messages for description.max and numeric validators on minLvl/maxLvl; update the signature to accept the i18n translator (t) and replace the validators with custom message options (e.g., description: z.string().max(200, { message: t('createForm.descriptionMax') }).optional(), and for minLvl/maxLvl use z.coerce.number().min(1, { message: t('createForm.levelMin') }).max(500, { message: t('createForm.levelMax') }).optional().or(z.literal("", { message: t('createForm.emptyLevel') })) ), keep the existing .refine for cross-field comparison but use t('createForm.levelRange') for its message, and add the corresponding keys to the createForm message bundle; update callers of getFormSchema to pass the t function. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/notifications/components/notifications-list.test.tsx-97-114 (1)</summary><blockquote> `97-114`: _⚠️ Potential issue_ | _🟡 Minor_ **Install the `scrollTo` spy before rendering.** This test patches `scrollTo` after `render`, so a mount-time scroll could already have happened and still pass unnoticed. Patch the prototype before rendering, then assert no calls. <details> <summary>🧪 Proposed test fix</summary> ```diff it("does not scroll on mount when there is no animation cycle", () => { + const scrollToSpy = vi.fn(); + Object.defineProperty(HTMLDivElement.prototype, "scrollTo", { + configurable: true, + value: scrollToSpy, + }); + render(<NotificationsList notifications={[createStoredNotification()]} />); const viewport = document.querySelector( "[data-radix-scroll-area-viewport]", ); @@ - const scrollToSpy = vi.fn(); - Object.defineProperty(viewport, "scrollTo", { - configurable: true, - value: scrollToSpy, - }); - expect(scrollToSpy).not.toHaveBeenCalled(); }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/notifications/components/notifications-list.test.tsx` around lines 97 - 114, The test currently patches viewport.scrollTo after render, so move the spy installation to before mounting: define a spy on HTMLDivElement.prototype.scrollTo (e.g., via Object.defineProperty(HTMLDivElement.prototype, "scrollTo", ...)) prior to calling render(<NotificationsList ... />) and then render the component with createStoredNotification; after render, query the viewport and assert the spy (scrollToSpy) was not called; ensure the prototype patch is configurable/restored after the test to avoid leaking into other tests. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/command/command.tsx-82-82 (1)</summary><blockquote> `82-82`: _⚠️ Potential issue_ | _🟡 Minor_ **Prefer `??` over `||` unless falsy-coalescing (empty string → undefined) is intentional.** Since `submissionMessage` is typed as `string` and can be empty (per tests), the current `|| undefined` will convert empty strings to `undefined` only for the party command while leaving it unchanged elsewhere. If this intentional distinction is needed, add a comment explaining the behavior; otherwise, switch to `??` to comply with guidelines and be consistent with other usages. ```diff - const description = submissionState.submissionMessage || undefined; + const description = submissionState.submissionMessage ?? undefined; ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/command/command.tsx` at line 82, The line setting description uses the || operator which turns an empty string into undefined; update the expression to use the nullish coalescing operator (submissionState.submissionMessage ?? undefined) to preserve empty strings unless the empty→undefined behavior is intentional—if it is intentional, add an explicit comment next to the assignment explaining why. Target the assignment that references submissionState.submissionMessage (the description variable) and make this change to match other usages. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (18)</summary><blockquote> <details> <summary>.agents/skills/stitch-design-taste/SKILL.md (1)</summary><blockquote> `84-84`: **Clarify the minimum body-text rule (`1rem` vs `14px`).** Line 84 mixes two different minima (`1rem` and `14px`). In standard setups, `1rem` is 16px, so this is ambiguous for downstream agents. <details> <summary>Suggested doc patch</summary> ```diff -- **Typography Scaling:** Headlines scale via `clamp()`. Body text minimum `1rem`/`14px` +- **Typography Scaling:** Headlines scale via `clamp()`. Body text minimum `1rem` (16px base), unless a non-default root size is explicitly defined. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/stitch-design-taste/SKILL.md at line 84, Clarify the ambiguous minimum body-text rule in the "Typography Scaling" bullet so it doesn't mix units: update the line that currently reads "**Typography Scaling:** Headlines scale via `clamp()`. Body text minimum `1rem`/`14px`" to state a single definite minimum (e.g., "Body text minimum 1rem (16px)" or "Body text minimum 14px (≈0.875rem)") or explicitly explain the environment where 1rem equals 14px; ensure the "Typography Scaling" bullet (the line mentioning clamp() and body text minimum) is updated accordingly. ``` </details> </blockquote></details> <details> <summary>.agents/skills/stitch-design-taste/DESIGN.md (1)</summary><blockquote> `83-83`: **Resolve conflicting typography minimums in this spec.** Line 83 says body text stays `1rem` minimum and also “never below `14px`”. This is internally inconsistent unless root font-size assumptions are specified. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/stitch-design-taste/DESIGN.md at line 83, The spec's "Typography Scaling" line conflicts by requiring body text to stay "1rem" minimum and also "Never shrink body below 14px"; resolve by choosing one consistent minimum and adding the root-font-size assumption: replace that sentence in the "Typography Scaling" section with a single rule such as "Body text minimum: 14px (0.875rem assuming 16px root)" or alternatively "Body text minimum: 1rem (assumes 16px root); do not go below 14px" and include the root font-size assumption and the use of clamp() for headlines (retain `clamp()` and the 375px readability requirement) so implementers know which baseline to use. ``` </details> </blockquote></details> <details> <summary>.agents/skills/gpt-taste/SKILL.md (2)</summary><blockquote> `72-72`: **Optional: Simplify redundant phrase.** Similar to line 43, the phrase "zero empty spaces" is flagged by static analysis as redundant. Consider "no empty spaces" or "zero spaces" for consistency. <details> <summary>✍️ Suggested simplification</summary> ```diff -4. **Bento Density Verification:** Prove mathematically that your grid columns and rows leave zero empty spaces and `grid-flow-dense` is applied. +4. **Bento Density Verification:** Prove mathematically that your grid columns and rows leave no empty spaces and `grid-flow-dense` is applied. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/gpt-taste/SKILL.md at line 72, Replace the redundant phrase "zero empty spaces" in the SKILL.md bullet ("4. **Bento Density Verification:** ... leave zero empty spaces and `grid-flow-dense` is applied") with a simpler form such as "no empty spaces" (or "zero spaces") for consistency with the earlier phrasing on line 43; update the bullet text to read e.g. "leave no empty spaces and `grid-flow-dense` is applied." ``` </details> --- `43-43`: **Optional: Simplify redundant phrase.** The phrase "Zero Empty Space" is flagged by static analysis as redundant. Consider "No Empty Space" or "Zero Space" for cleaner phrasing. <details> <summary>✍️ Suggested simplification</summary> ```diff -- **Zero Empty Space in Grids:** LLMs notoriously leave blank, dead cells in CSS grids. +- **No Empty Space in Grids:** LLMs notoriously leave blank, dead cells in CSS grids. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/gpt-taste/SKILL.md at line 43, The header phrase "Zero Empty Space in Grids" is flagged as redundant; update the sentence in SKILL.md to use a simpler phrase such as "No Empty Space in Grids" (or "Zero Space in Grids") while keeping the required instruction intact: retain references to Tailwind's `grid-flow-dense` (`grid-auto-flow: dense`) and the requirement to verify `col-span` and `row-span` interlock for every Bento Grid so no empty cells remain. Ensure the rest of the sentence and symbols (`grid-flow-dense`, `col-span`, `row-span`, Bento Grid) are unchanged. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/command/command-state.test.ts (1)</summary><blockquote> `27-38`: **Add a regression case for `!grp` token boundaries.** Please cover inputs like `!grupa` or `!grpfoo` so the parser does not accidentally classify non-`!grp` commands as party mode. <details> <summary>🧪 Proposed test</summary> ```diff it("extracts the !grp description and keeps the party mode", () => { expect( getCommandState("!grp szukam tanca", { selectedGuildCount: 1 }), ).toMatchObject({ mode: "party", isNotification: true, isPartyCommand: true, hasContent: true, canSubmit: true, submissionMessage: "szukam tanca", }); }); + + it("does not treat words starting with !grp as the party command", () => { + expect(getCommandState("!grupa", { selectedGuildCount: 1 })).toMatchObject({ + mode: "notification", + isNotification: true, + isPartyCommand: false, + submissionMessage: "grupa", + }); + }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/command/command-state.test.ts` around lines 27 - 38, Add regression tests in the same test suite for getCommandState to ensure token boundaries are respected: add cases like getCommandState("!grupa something", { selectedGuildCount: 1 }) and getCommandState("!grpfoo", { selectedGuildCount: 1 }) and assert that isPartyCommand is false (and mode is not "party") while the submissionMessage/hasContent reflect the full input (i.e., the parser should not strip the leading token as a party command); place these new it() blocks alongside the existing "!grp" test so getCommandState is explicitly verified to reject "!grupa" and "!grpfoo" as party commands. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/settings/components/timers/components/hidden-colors-list.tsx (1)</summary><blockquote> `37-37`: **Prefer `??` over `||` for the fallback.** `colorNames[colorId] || getDefaultColorName(colorId)` will also fall back when the custom name is an empty string. If that is actually desired, `||` is fine; otherwise switch to `??` so only `undefined`/`null` trigger the default — this also matches the repo style guide. <details> <summary>Proposed change</summary> ```diff - {colorNames[colorId] || getDefaultColorName(colorId)} + {colorNames[colorId] ?? getDefaultColorName(colorId)} ``` </details> As per coding guidelines: "Prefer `??` over `||` for nullish coalescing". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/settings/components/timers/components/hidden-colors-list.tsx` at line 37, The expression using logical OR should use nullish coalescing so empty strings are preserved: replace the fallback expression in HiddenColorsList (where colorNames[colorId] is used with getDefaultColorName(colorId)) to use ?? instead of ||; update the JSX line that references colorNames, colorId, and getDefaultColorName so it reads as a nullish coalescing fallback (only null/undefined fall back to getDefaultColorName). ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/notifications/components/single-notification-message.test.tsx (1)</summary><blockquote> `8-20`: **Consider a typed factory instead of `as never` for the notification fixture.** Casting the partial object via `as never` bypasses the `notification` prop's type contract entirely. If the `Notification` shape changes (new required field, renamed key), this test will silently keep compiling while the production component may break at runtime. A small typed fixture builder (e.g. `buildNotification(overrides)`) returning the real type would surface such drift during refactors and makes the test more resilient. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/notifications/components/single-notification-message.test.tsx` around lines 8 - 20, Replace the unsafe "as never" test fixture with a typed factory: implement a buildNotification(overrides?: Partial<Notification>): Notification helper that returns a fully populated Notification object (provide sensible defaults for notificationId, discordId, guildId, world, createdAt, message, servers, etc.), then use SingleNotificationMessage with notification={buildNotification({ notificationId: "message-1", ... })}; this ensures the fixture matches the actual Notification type and surfaces type drift in SingleNotificationMessage's props. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/timers/components/timers-filters.tsx (1)</summary><blockquote> `185-185`: **Prefer `??` over `||` for the fallback.** Per coding guidelines, use nullish coalescing. Empty-string default color names (unlikely but possible) would otherwise be unintentionally replaced by the fallback. <details> <summary>♻️ Proposed change</summary> ```diff - {defaultColorNames[colorId] || getDefaultColorName(colorId)} + {defaultColorNames[colorId] ?? getDefaultColorName(colorId)} ``` </details> As per coding guidelines: "Prefer `??` over `||` for nullish coalescing". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/timers/components/timers-filters.tsx` at line 185, Replace the logical-OR fallback with nullish-coalescing in the render expression: in the component that uses defaultColorNames and getDefaultColorName (the expression referencing defaultColorNames[colorId] || getDefaultColorName(colorId)), change the fallback to use ?? so empty-string or falsey but non-null/undefined values from defaultColorNames[colorId] are preserved (i.e., use defaultColorNames[colorId] ?? getDefaultColorName(colorId)). ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/timers/utils/color-statistics.ts (1)</summary><blockquote> `47-47`: **Prefer `??` over `||` for fallback chain.** Guideline-aligned nullish coalescing avoids collapsing accidental empty-string names. <details> <summary>♻️ Proposed change</summary> ```diff - name: defaultColorNames[color] || getDefaultColorName(color) || color, + name: defaultColorNames[color] ?? getDefaultColorName(color) ?? color, ``` </details> As per coding guidelines: "Prefer `??` over `||` for nullish coalescing". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/timers/utils/color-statistics.ts` at line 47, Replace the logical-OR fallbacks in the color name assignment to use nullish coalescing so empty-string names are preserved: change the expression that sets the name (which currently uses defaultColorNames[color] || getDefaultColorName(color) || color) to use ?? between defaultColorNames[color], getDefaultColorName(color), and color, referencing the same identifiers (defaultColorNames and getDefaultColorName) so the fallback chain only triggers for null or undefined. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/timers/components/timer-color-picker.tsx (1)</summary><blockquote> `71-71`: **Prefer `??` over `||` for the fallback.** <details> <summary>♻️ Proposed change</summary> ```diff - {defaultColorNames[id] || getDefaultColorName(id)} + {defaultColorNames[id] ?? getDefaultColorName(id)} ``` </details> As per coding guidelines: "Prefer `??` over `||` for nullish coalescing". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/timers/components/timer-color-picker.tsx` at line 71, Replace the logical-or fallback with nullish coalescing: in the JSX expression that renders the color name (using defaultColorNames and getDefaultColorName with id), change the fallback operator from || to ?? so that undefined/null values fall back to getDefaultColorName(id) while preserving falsy but valid values like empty string or 0. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/notifications/components/single-notification-npc.test.tsx (1)</summary><blockquote> `9-32`: **Replace `as never` test fixtures with properly-typed fixtures.** The casts at lines 32 and 56 bypass type checking for the `SingleNotificationNpc` component props. If the notification shape changes, these tests will still compile with stale fixtures. Instead, extract the prop type and assign fixtures to that type so incompatibilities surface at test time. <details> <summary>Example approach</summary> ```diff +import type { ComponentProps } from "react"; import { render, screen } from "@testing-library/react"; import { describe, expect, it } from "vitest"; import { SingleNotificationNpc } from "./single-notification-npc"; + +type NotificationProp = ComponentProps< + typeof SingleNotificationNpc +>["notification"]; ``` Then declare each fixture as `NotificationProp` instead of casting inline. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/notifications/components/single-notification-npc.test.tsx` around lines 9 - 32, Replace the inline "as never" casts with properly‑typed fixtures by extracting the notification prop type from the SingleNotificationNpc component (e.g., via ComponentProps<typeof SingleNotificationNpc>['notification'] or the exported prop type) and declare each test fixture variable with that type, then assign the object literal to the typed variable and use that variable in the test render; remove the "as never" casts so TypeScript will catch any shape mismatches between the fixture and SingleNotificationNpc's props. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/command/components/command-suggestions.tsx (1)</summary><blockquote> `4-70`: **Remove `useMemo` here.** `COMMAND_SUGGESTIONS` is tiny, and the repository’s React Compiler policy disallows manual React memoization in TSX. <details> <summary>♻️ Proposed refactor</summary> ```diff -import { useEffect, useMemo, useState, type FC } from "react"; +import { useEffect, useState, type FC } from "react"; @@ - const filtered = useMemo( - () => - COMMAND_SUGGESTIONS.filter((suggestion) => - suggestion.prefix.startsWith(commandQuery), - ).sort((left, right) => { - const leftExactMatch = left.prefix === commandQuery ? 1 : 0; - const rightExactMatch = right.prefix === commandQuery ? 1 : 0; + const filtered = COMMAND_SUGGESTIONS.filter((suggestion) => + suggestion.prefix.startsWith(commandQuery), + ).sort((left, right) => { + const leftExactMatch = left.prefix === commandQuery ? 1 : 0; + const rightExactMatch = right.prefix === commandQuery ? 1 : 0; - if (leftExactMatch !== rightExactMatch) { - return rightExactMatch - leftExactMatch; - } + if (leftExactMatch !== rightExactMatch) { + return rightExactMatch - leftExactMatch; + } - return left.prefix.length - right.prefix.length; - }), - [commandQuery], - ); + return left.prefix.length - right.prefix.length; + }); ``` </details> As per coding guidelines, “React Compiler handles memoization — do not use `memo`, `useMemo`, or `useCallback`”. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/command/components/command-suggestions.tsx` around lines 4 - 70, The code uses useMemo to compute filtered suggestions inside useCommandSuggestions which violates the React Compiler policy; replace the memoized computation with a plain synchronous computation: remove useMemo import from the top, change the filtered declaration to a simple const filtered = COMMAND_SUGGESTIONS.filter(...).sort(...) (keeping the same filter/sort logic and dependency on commandQuery), and ensure useCommandSuggestions, COMMAND_SUGGESTIONS, and the filtered variable remain otherwise unchanged so selection and rendering logic still work. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/command/components/command-context-panel.tsx (1)</summary><blockquote> `36-44`: **Replace the nested ternary with explicit branching.** The recipient-details logic is easier to read and maintain as `if` branches, especially with localized overflow text. <details> <summary>♻️ Proposed refactor</summary> ```diff - const recipientsDetails = - selectedGuildNames.length > 0 - ? hiddenGuildCount > 0 - ? t("settings.command.recipients.selectedNamesOverflow", { - names: visibleGuildNames, - count: hiddenGuildCount, - }) - : visibleGuildNames - : t("settings.command.recipients.missingHint"); + let recipientsDetails = t("settings.command.recipients.missingHint"); + + if (selectedGuildNames.length > 0) { + recipientsDetails = visibleGuildNames; + } + + if (hiddenGuildCount > 0) { + recipientsDetails = t("settings.command.recipients.selectedNamesOverflow", { + names: visibleGuildNames, + count: hiddenGuildCount, + }); + } ``` </details> As per coding guidelines, “Avoid nested (chained) ternary expressions — use early returns or `if`/`else if` instead”. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/command/components/command-context-panel.tsx` around lines 36 - 44, The nested ternary that computes recipientsDetails should be rewritten using explicit if/else branching: check selectedGuildNames.length > 0 first, then inside that block check hiddenGuildCount > 0 and set recipientsDetails to t("settings.command.recipients.selectedNamesOverflow", { names: visibleGuildNames, count: hiddenGuildCount }) or to visibleGuildNames accordingly; otherwise set it to t("settings.command.recipients.missingHint"). Update the logic around selectedGuildNames, hiddenGuildCount, visibleGuildNames, and the t(...) calls in command-context-panel.tsx (where recipientsDetails is defined) to replace the nested ternary with clear if / else if / else statements. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/store/notifications.store.test.ts (1)</summary><blockquote> `73-81`: **Add `afterEach` to restore real timers.** `vi.useFakeTimers()` is installed in `beforeEach` but never reset. If this suite runs in the same worker as files that assume real timers (and don't explicitly switch), timing-sensitive tests there can become flaky. <details> <summary>♻️ Proposed cleanup</summary> ```diff beforeEach(() => { vi.useFakeTimers(); vi.setSystemTime(new Date("2026-04-19T10:00:00.000Z")); ... }); + + afterEach(() => { + vi.useRealTimers(); + }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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 73 - 81, Add an afterEach that restores real timers to complement the beforeEach that calls vi.useFakeTimers(); specifically, add an afterEach hook that calls vi.useRealTimers() (or vi.clearAllTimers() then vi.useRealTimers()) so tests that assume real timers are not affected; update the test file's suite surrounding beforeEach/vi.useFakeTimers to include this afterEach to ensure timers are reset between tests. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/npc-detector/components/npc-list-item.test.tsx (2)</summary><blockquote> `144-198`: **Restore stubbed globals and clean up spies to prevent cross-test leakage.** A few cleanup gaps: 1. `vi.stubGlobal("message", vi.fn())` at line 197 is never unstubbed — `window.message` will bleed into any subsequent tests in the same worker. 2. `vi.spyOn(console, "error")...mockImplementation(...)` at line 341-343 isn't restored; `vi.clearAllMocks()` does not restore original implementations. <details> <summary>♻️ Proposed cleanup</summary> ```diff beforeEach(() => { vi.clearAllMocks(); vi.useRealTimers(); ... vi.stubGlobal("message", vi.fn()); }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); ``` Note: if you add `vi.restoreAllMocks()`, verify that the module-level `vi.fn()`s declared outside `describe` are still usable across tests (they are — `vi.restoreAllMocks` only restores `vi.spyOn` targets, not standalone `vi.fn()`s). </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/npc-detector/components/npc-list-item.test.tsx` around lines 144 - 198, Add an afterEach cleanup that unstubs globals and restores spies: call vi.unstubAllGlobals() (to remove the vi.stubGlobal("message", ...) global) and vi.restoreAllMocks() (to restore any vi.spyOn console.error mock and similar spies); place this in the same describe("NpcListItem", ...) block so it runs after each test and prevents cross-test leakage while keeping module-level vi.fn() mocks intact. ``` </details> --- `243-311`: **Index-based button lookups are brittle — prefer accessible name/role queries.** `screen.getAllByRole("button")[0]` and `[1]` (lines 243, 308-309, 354-355) depend on DOM insertion order and will silently break if a neutral button (e.g., tooltip trigger, icon button) is inserted before the send/gather controls. Prefer an accessible name query so the assertion fails with a clear message: ```diff - const [sendButton] = screen.getAllByRole("button"); + const sendButton = screen.getByRole("button", { name: /wyślij|send/i }); ``` Apply the same approach to the `gatherButton` selection. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/npc-detector/components/npc-list-item.test.tsx` around lines 243 - 311, Replace brittle index-based button lookups in npc-list-item.test.tsx (the uses of screen.getAllByRole("button")[0] assigned to sendButton and screen.getAllByRole("button")[1] assigned to gatherButton) with accessible role+name queries: locate the send and gather controls via screen.getByRole('button', { name: /send/i }) and screen.getByRole('button', { name: /gather/i }) (or the exact accessible labels used in the UI), and update the tests that reference sendButton and gatherButton accordingly so they fail clearly if the labeled buttons are missing. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/command/command.tsx (1)</summary><blockquote> `59-65`: **Duplicated `onSelect` handler — extract once and reuse.** The inline `(prefix) => { setValue("message", prefix); textareaRef.current?.focus(); }` appears in both `useCommandSuggestions` (line 61-64) and `<CommandSuggestions .../>` (line 183-186). Extract a single handler to avoid drift. <details> <summary>♻️ Proposed refactor</summary> ```diff + const handleSuggestionSelect = (prefix: string) => { + setValue("message", prefix); + textareaRef.current?.focus(); + }; + const suggestions = useCommandSuggestions({ inputValue: messageValue, - onSelect: (prefix) => { - setValue("message", prefix); - textareaRef.current?.focus(); - }, + onSelect: handleSuggestionSelect, }); @@ <CommandSuggestions - onSelect={(prefix) => { - setValue("message", prefix); - textareaRef.current?.focus(); - }} + onSelect={handleSuggestionSelect} ``` </details> Also applies to: 181-190 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/command/command.tsx` around lines 59 - 65, Extract the duplicated onSelect handler into a single function and use it in both places: define a const handleSelect = (prefix: string) => { setValue("message", prefix); textareaRef.current?.focus(); } near the component scope (so it can access setValue and textareaRef), then pass handleSelect to useCommandSuggestions(...) and as the onSelect prop on <CommandSuggestions .../>; ensure the function name is unique (handleSelect) and that both references replace the inline arrow handlers to prevent drift. ``` </details> </blockquote></details> <details> <summary>apps/game-client/src/features/notifications/components/single-notification.test.tsx (1)</summary><blockquote> `186-241`: **Add an `afterEach` to restore timers and the `SVGElement.prototype.animate` override.** `vi.useFakeTimers()` and `Object.defineProperty(SVGElement.prototype, "animate", ...)` are set in `beforeEach` without teardown. The prototype override persists across tests/files in the same worker and fake timers remain active if a subsequent file doesn't set them explicitly — both can introduce flaky behavior in unrelated suites. <details> <summary>♻️ Proposed cleanup</summary> ```diff describe("SingleNotification", () => { + const originalAnimate = Object.getOwnPropertyDescriptor( + SVGElement.prototype, + "animate", + ); + beforeEach(() => { ... }); + + afterEach(() => { + vi.useRealTimers(); + if (originalAnimate) { + Object.defineProperty(SVGElement.prototype, "animate", originalAnimate); + } else { + // `@ts-expect-error` removing the stub + delete SVGElement.prototype.animate; + } + }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@apps/game-client/src/features/notifications/components/single-notification.test.tsx` around lines 186 - 241, Add an afterEach that calls vi.useRealTimers() to restore real timers and removes/restores the SVG animate override set in beforeEach (undo Object.defineProperty on SVGElement.prototype.animate, e.g. delete SVGElement.prototype.animate or restore the original value captured before override), referencing the mockAnimate used in beforeEach and the vi.useFakeTimers()/vi.useRealTimers() calls and SVGElement.prototype.animate to ensure no prototype or timer leaks across tests. ``` </details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| * **COLOR CONSISTENCY:** Stick to one palette for the entire output. Do not fluctuate between warm and cool grays within the same project. | ||
|
|
||
| **Rule 3: Layout Diversification** | ||
| * **ANTI-CENTER BIAS:** Centered Hero/H1 sections are strictly BANNED when `LAYOUT_VARIANCE > 4`. Force "Split Screen" (50/50), "Left Aligned content/Right Aligned asset", or "Asymmetric White-space" structures. |
There was a problem hiding this comment.
Fix config variable mismatch (LAYOUT_VARIANCE vs DESIGN_VARIANCE).
Line 50 references LAYOUT_VARIANCE, but the defined baseline variable is DESIGN_VARIANCE (Line 9). This creates conflicting control logic in the spec.
Suggested fix
-* **ANTI-CENTER BIAS:** Centered Hero/H1 sections are strictly BANNED when `LAYOUT_VARIANCE > 4`. Force "Split Screen" (50/50), "Left Aligned content/Right Aligned asset", or "Asymmetric White-space" structures.
+* **ANTI-CENTER BIAS:** Centered Hero/H1 sections are strictly BANNED when `DESIGN_VARIANCE > 4`. Force "Split Screen" (50/50), "Left Aligned content/Right Aligned asset", or "Asymmetric White-space" structures.📝 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
| * **ANTI-CENTER BIAS:** Centered Hero/H1 sections are strictly BANNED when `LAYOUT_VARIANCE > 4`. Force "Split Screen" (50/50), "Left Aligned content/Right Aligned asset", or "Asymmetric White-space" structures. | |
| * **ANTI-CENTER BIAS:** Centered Hero/H1 sections are strictly BANNED when `DESIGN_VARIANCE > 4`. Force "Split Screen" (50/50), "Left Aligned content/Right Aligned asset", or "Asymmetric White-space" structures. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/design-taste-frontend/SKILL.md at line 50, The rule
references the wrong config variable: replace the `LAYOUT_VARIANCE` token used
in the "ANTI-CENTER BIAS" rule with the actual baseline variable
`DESIGN_VARIANCE` (or alternatively rename the baseline `DESIGN_VARIANCE` to
`LAYOUT_VARIANCE` everywhere) so the condition is consistent; update the rule
text mentioning `LAYOUT_VARIANCE > 4` to `DESIGN_VARIANCE > 4` (or update the
baseline name) and ensure the same symbol (`DESIGN_VARIANCE` or
`LAYOUT_VARIANCE`) is used throughout the document, including the
header/baseline definition and every rule that references it.
Comment on lines
+69
to
+75
| ## 7. Subtle Motion & Micro-Animations | ||
| Motion should feel invisible — present but never distracting. The goal is quiet sophistication, not spectacle. | ||
| - Scroll Entry: Elements fade in gently as they enter the viewport. Use `translateY(12px)` + `opacity: 0` resolving over `600ms` with `cubic-bezier(0.16, 1, 0.3, 1)`. Use `IntersectionObserver`, never `window.addEventListener('scroll')`. | ||
| - Hover States: Cards lift with an ultra-subtle shadow shift (`box-shadow` transitioning from `0 0 0` to `0 2px 8px rgba(0,0,0,0.04)` over `200ms`). Buttons respond with `scale(0.98)` on `:active`. | ||
| - Staggered Reveals: Lists and grid items enter with a cascade delay (`animation-delay: calc(var(--index) * 80ms)`). Never mount everything at once. | ||
| - Background Ambient Motion: Optional. A single, very slow-moving radial gradient blob (`animation-duration: 20s+`, `opacity: 0.02-0.04`) drifting behind hero sections. Must be applied to a `position: fixed; pointer-events: none` layer. Never on scrolling containers. | ||
| - Performance: Animate exclusively via `transform` and `opacity`. No layout-triggering properties (`top`, `left`, `width`, `height`). Use `will-change: transform` sparingly and only on actively animating elements. |
There was a problem hiding this comment.
Include reduced-motion handling in motion protocol requirements.
The motion section prescribes scroll-entry, stagger, and ambient animation patterns, but doesn’t specify behavior for users requesting reduced motion. Add explicit prefers-reduced-motion guidance so generated implementations are accessibility-safe by default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/minimalist-ui/SKILL.md around lines 69 - 75, Add explicit
prefers-reduced-motion guidance: for each motion rule (Scroll Entry, Hover
States, Staggered Reveals, Background Ambient Motion) require honoring `@media`
(prefers-reduced-motion: reduce) or a runtime reduce-motion flag so animations
are suppressed — e.g., skip IntersectionObserver entry animations and render
elements fully visible (opacity:1, transform:none), remove stagger delays
(animation-delay:0), disable ambient gradient animation, and prevent
hover/active transforms; recommend using a minimal non-animating fallback
(instant transition or 0ms) and document that implementations must check
prefers-reduced-motion before applying translateY/opacity/animation-duration
effects.
Comment on lines
+67
to
+75
| - **Instant transitions with zero duration.** Add smooth transitions (200-300ms) to all interactive elements. | ||
| - **Missing focus ring.** Ensure visible focus indicators for keyboard navigation. This is an accessibility requirement, not optional. | ||
| - **No loading states.** Replace generic circular spinners with skeleton loaders that match the layout shape. | ||
| - **No empty states.** An empty dashboard showing nothing is a missed opportunity. Design a composed "getting started" view. | ||
| - **No error states.** Add clear, inline error messages for forms. Do not use `window.alert()`. | ||
| - **Dead links.** Buttons that link to `#`. Either link to real destinations or visually disable them. | ||
| - **No indication of current page in navigation.** Style the active nav link differently so users know where they are. | ||
| - **Scroll jumping.** Anchor clicks jump instantly. Add `scroll-behavior: smooth`. | ||
| - **Animations using `top`, `left`, `width`, `height`.** Switch to `transform` and `opacity` for GPU-accelerated, smooth animation. |
There was a problem hiding this comment.
Add reduced-motion exceptions for animation and smooth scrolling directives.
Line 67 and Line 74 enforce motion globally, but there is no prefers-reduced-motion fallback. This can violate accessibility expectations and produce a poor UX for motion-sensitive users.
Suggested doc patch
- **Instant transitions with zero duration.** Add smooth transitions (200-300ms) to all interactive elements.
+ - Respect reduced-motion preferences: disable or minimize non-essential transitions under `@media (prefers-reduced-motion: reduce)`.
...
- **Scroll jumping.** Anchor clicks jump instantly. Add `scroll-behavior: smooth`.
+ - Apply smooth scrolling only when reduced motion is not requested.🧰 Tools
🪛 LanguageTool
[grammar] ~67-~67: Ensure spelling is correct
Context: ...duration.** Add smooth transitions (200-300ms) to all interactive elements. - **Missi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/redesign-existing-projects/SKILL.md around lines 67 - 75, The
doc currently mandates global motion (e.g., "scroll-behavior: smooth" and
animation guidance to use "transform" and "opacity") but omits a
prefers-reduced-motion fallback; update the guidance to require adding a CSS
prefers-reduced-motion media query that disables or reduces non-essential
animations/transitions and overrides smooth scrolling for motion-sensitive users
(e.g., recommend using `@media` (prefers-reduced-motion: reduce) to set
animations/transition-duration to 0ms or minimal and to reset scroll-behavior to
auto), and note that interactive components (focus rings, skeleton loaders,
active nav styling) should respect this reduced-motion setting too (refer to
"scroll-behavior: smooth", "transform and opacity", and "prefers-reduced-motion"
in the doc).
Comment on lines
+95
to
+99
| - **Perpetual Micro-Loops:** Every active dashboard component has an infinite-loop state — Pulse on status dots, Typewriter on search bars, Float on feature icons, Shimmer on loading states | ||
| - **Staggered Orchestration:** Lists and grids mount with cascaded delays (`animation-delay: calc(var(--index) * 100ms)`). Waterfall reveals, never instant mount | ||
| - **Layout Transitions:** Smooth re-ordering via shared element IDs. Items swap positions with physics, simulating real-time intelligence | ||
| - **Hardware Rules:** Animate ONLY `transform` and `opacity`. Never `top`, `left`, `width`, `height`. Grain/noise filters on fixed, pointer-events-none pseudo-elements only | ||
| - **Performance:** CPU-heavy perpetual animations isolated in microscopic leaf components. Never trigger parent re-renders. Target 60fps minimum |
There was a problem hiding this comment.
Add reduced-motion fallback to motion intent requirements.
This section mandates infinite-loop behaviors and rich motion patterns without a reduced-motion path. The doc should explicitly require a prefers-reduced-motion variant.
🧰 Tools
🪛 LanguageTool
[grammar] ~99-~99: Ensure spelling is correct
Context: ...Never trigger parent re-renders. Target 60fps minimum ## 9. Anti-Patterns (Banned) -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~99-~99: Ensure spelling is correct
Context: ... parent re-renders. Target 60fps minimum ## 9. Anti-Patterns (Banned) - No emojis — ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/stitch-design-taste/DESIGN.md around lines 95 - 99, Update
the motion requirements in the DESIGN.md section (items "Perpetual Micro-Loops",
"Staggered Orchestration", "Layout Transitions", and "Hardware Rules") to
mandate a prefers-reduced-motion fallback: add language that infinite-loop
effects (Pulse, Typewriter, Float, Shimmer) must have a non-animating
alternative state, staggered mounts using animation-delay (calc(var(--index) *
100ms)) must collapse to instantaneous or sequenced appearance, and
shared-element layout transitions must degrade to immediate swaps when
prefers-reduced-motion is set; also require CSS guidance to wrap heavy
animations in a `@media` (prefers-reduced-motion: reduce) rule that disables or
replaces transform/opacity effects and prevents CPU-heavy micro-components from
animating while still preserving accessibility.
Comment on lines
+92
to
+95
| - **Perpetual Micro-Interactions:** Every active component should have an infinite loop state (Pulse, Typewriter, Float, Shimmer) | ||
| - **Staggered Orchestration:** Never mount lists instantly — use cascade delays for waterfall reveals | ||
| - **Performance:** Animate exclusively via `transform` and `opacity`. Never animate `top`, `left`, `width`, `height`. Grain/noise filters on fixed pseudo-elements only | ||
|
|
There was a problem hiding this comment.
Avoid unconditional infinite-loop animation requirements.
Line 92 currently mandates perpetual loops for every active component. Add a reduced-motion exception so generated implementations remain accessible.
Suggested doc patch
-- **Perpetual Micro-Interactions:** Every active component should have an infinite loop state (Pulse, Typewriter, Float, Shimmer)
+- **Perpetual Micro-Interactions:** Active components may use looped states (Pulse, Typewriter, Float, Shimmer) when motion is appropriate.
+ - Under `prefers-reduced-motion: reduce`, disable perpetual loops and keep only essential state changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/stitch-design-taste/SKILL.md around lines 92 - 95, Update the
"Perpetual Micro-Interactions" rule so infinite-loop animations like Pulse,
Typewriter, Float, Shimmer include a reduced-motion opt-out: detect
prefers-reduced-motion (or a runtime reducedMotion flag) and disable or replace
loops with a single static state or subtle non-animating fallback; reference the
"Perpetual Micro-Interactions" heading and the listed animation types (Pulse,
Typewriter, Float, Shimmer) when changing the wording to require accessible
fallbacks that honor reduced-motion preferences.
Comment on lines
+33
to
+39
| expect(screen.getByText("Tester")).toBeInTheDocument(); | ||
| expect(screen.getByText("(240w)")).toBeInTheDocument(); | ||
| expect(screen.getByText("Szuka grupy")).toBeInTheDocument(); | ||
| expect(screen.getByText('"Brakuje tanka"')).toBeInTheDocument(); | ||
| expect(screen.getByText("Poziom: 200 - 260")).toHaveClass( | ||
| "ll:text-gray-300", | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd single-notification-party-gathering.tsx --type f --exec cat {}
rg -nP 'Szuka grupy|Poziom:' -C2Repository: lootlog/monorepo
Length of output: 4936
SingleNotificationPartyGathering hardcodes Polish strings in violation of the i18n guideline.
The component contains hardcoded Polish text:
- Line 23:
"Szuka grupy" - Line 39:
"Poziom: {minLvl} - {maxLvl}"
This violates the coding guideline: "All static text in frontend apps must use i18n — never hardcode user-facing strings."
An i18n translation already exists in chat.json under partyGathering.levelLabel for the level label. Refactor the component to use useTranslation() and replace hardcoded strings with t() calls, then update test assertions to work with the i18n provider.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/game-client/src/features/notifications/components/single-notification-party-gathering.test.tsx`
around lines 33 - 39, The tests and component use hardcoded Polish strings;
update the SingleNotificationPartyGathering component to import and use
useTranslation() and replace hardcoded labels ("Szuka grupy" and the level label
"Poziom: {minLvl} - {maxLvl}") with t('chat:partyGathering.someKey') (use the
existing chat.json key partyGathering.levelLabel for the level label and add
keys for the other strings if needed), then update tests in
single-notification-party-gathering.test.tsx to render the component with an
i18n provider (or mock useTranslation/t to return expected strings) and change
assertions to look up the translated text rather than hardcoded Polish text;
locate code by the SingleNotificationPartyGathering component and the test file
to apply these changes.
Comment on lines
+80
to
+98
| it("does not scroll on mount when there is no detection cycle yet", () => { | ||
| render(<NpcsList npcs={[createNpc()]} />); | ||
|
|
||
| const viewport = document.querySelector( | ||
| "[data-radix-scroll-area-viewport]", | ||
| ); | ||
|
|
||
| if (!(viewport instanceof HTMLDivElement)) { | ||
| throw new Error("Expected scroll viewport"); | ||
| } | ||
|
|
||
| const scrollToSpy = vi.fn(); | ||
| Object.defineProperty(viewport, "scrollTo", { | ||
| configurable: true, | ||
| value: scrollToSpy, | ||
| }); | ||
|
|
||
| expect(scrollToSpy).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
This test is tautological — the spy is installed after mount, so it cannot observe mount behavior.
scrollToSpy is created and assigned to viewport.scrollTo after render(...) has already completed. If the component did call scrollTo on mount, it would have invoked the element's original method, not the spy. The final expect(scrollToSpy).not.toHaveBeenCalled() only proves that a freshly-created vi.fn() was never called — which is trivially true.
To actually assert "no scroll on mount", the spy must be installed before render. One approach:
🛠 Proposed fix
it("does not scroll on mount when there is no detection cycle yet", () => {
- render(<NpcsList npcs={[createNpc()]} />);
-
- const viewport = document.querySelector(
- "[data-radix-scroll-area-viewport]",
- );
-
- if (!(viewport instanceof HTMLDivElement)) {
- throw new Error("Expected scroll viewport");
- }
-
- const scrollToSpy = vi.fn();
- Object.defineProperty(viewport, "scrollTo", {
- configurable: true,
- value: scrollToSpy,
- });
-
- expect(scrollToSpy).not.toHaveBeenCalled();
+ const scrollToSpy = vi.fn();
+ const originalScrollTo = HTMLDivElement.prototype.scrollTo;
+ HTMLDivElement.prototype.scrollTo = scrollToSpy;
+
+ try {
+ render(<NpcsList npcs={[createNpc()]} />);
+ expect(scrollToSpy).not.toHaveBeenCalled();
+ } finally {
+ HTMLDivElement.prototype.scrollTo = originalScrollTo;
+ }
});The sibling test at lines 100-127 has the same ordering issue; it happens to work only because the scrollTo is triggered by a later store update, but the setup pattern is fragile and should be made consistent.
📝 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
| it("does not scroll on mount when there is no detection cycle yet", () => { | |
| render(<NpcsList npcs={[createNpc()]} />); | |
| const viewport = document.querySelector( | |
| "[data-radix-scroll-area-viewport]", | |
| ); | |
| if (!(viewport instanceof HTMLDivElement)) { | |
| throw new Error("Expected scroll viewport"); | |
| } | |
| const scrollToSpy = vi.fn(); | |
| Object.defineProperty(viewport, "scrollTo", { | |
| configurable: true, | |
| value: scrollToSpy, | |
| }); | |
| expect(scrollToSpy).not.toHaveBeenCalled(); | |
| }); | |
| it("does not scroll on mount when there is no detection cycle yet", () => { | |
| const scrollToSpy = vi.fn(); | |
| const originalScrollTo = HTMLDivElement.prototype.scrollTo; | |
| HTMLDivElement.prototype.scrollTo = scrollToSpy; | |
| try { | |
| render(<NpcsList npcs={[createNpc()]} />); | |
| expect(scrollToSpy).not.toHaveBeenCalled(); | |
| } finally { | |
| HTMLDivElement.prototype.scrollTo = originalScrollTo; | |
| } | |
| }); |
🤖 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/components/npcs-list.test.tsx`
around lines 80 - 98, The test is tautological because scrollToSpy is attached
after render; install the spy on the element prototype before mounting so
mount-time calls are observed: create a scrollToSpy and
Object.defineProperty(HTMLDivElement.prototype, "scrollTo", { configurable:
true, value: scrollToSpy }) (or Element.prototype.scrollTo) prior to calling
render(<NpcsList npcs={[createNpc()]} />), then render and assert scrollToSpy
was not called, and finally restore the original scrollTo; apply the same change
to the sibling test that also installs the spy too late.
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
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