Feature/notifications enhancements#743
Merged
kamilwronka merged 12 commits intodevelopfrom Apr 17, 2026
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 238 files, which is 88 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (62)
📒 Files selected for processing (238)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR enhances notification-related UX and moves several client-side settings and API calls toward centralized backend-backed preferences, while adding action logging and improved settings UI structure.
Changes:
- Introduces backend-synced preference hooks (game-account notification/detector settings + user notification mutes) and sync-status UI.
- Refactors many hooks to use a centralized
@/apimodule (with logging wrappers) instead of ad-hoc axios client usage. - Adds new settings UI building blocks (layout/sections/panels), i18n wiring in several settings tabs, and an in-app error boundary window.
Reviewed changes
Copilot reviewed 180 out of 241 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/game-client/src/hooks/use-notification-settings-sync-status.tsx | Adds hook to derive loading/saving/error status for backend-synced notification settings. |
| apps/game-client/src/hooks/use-initial-configuration.tsx | Removes legacy local initialization for notification/detector settings. |
| apps/game-client/src/hooks/use-current-user-notification-mutes.tsx | Adds hook to derive effective notification mutes from user preferences. |
| apps/game-client/src/hooks/use-current-game-account-preferences.test.tsx | Adds tests ensuring default effective settings when fetched data is undefined. |
| apps/game-client/src/hooks/use-current-game-account-notification-settings.tsx | Adds hook to fetch/derive effective game-account notification settings. |
| apps/game-client/src/hooks/use-current-game-account-detector-settings.tsx | Adds hook to fetch/derive effective game-account detector settings. |
| apps/game-client/src/hooks/game-events/types.ts | Updates processed NPC setting type to shared @lootlog/types. |
| apps/game-client/src/hooks/auth/use-auth-token.tsx | Switches auth token fetching to centralized API function. |
| apps/game-client/src/hooks/auth/use-auth-scopes.tsx | Switches auth scopes fetching to centralized API function. |
| apps/game-client/src/hooks/api/use-worlds.tsx | Refactors worlds query to use fetchGuildWorlds. |
| apps/game-client/src/hooks/api/use-volunteer.ts | Refactors volunteer mutation to use centralized API. |
| apps/game-client/src/hooks/api/use-user.tsx | Refactors current-user query to use centralized API and exported User type. |
| apps/game-client/src/hooks/api/use-user-preferences.tsx | Adds hooks for fetching/updating user preferences with optimistic updates. |
| apps/game-client/src/hooks/api/use-update-lootlog-characters-config.tsx | Refactors config update to centralized API and updates cache shape. |
| apps/game-client/src/hooks/api/use-update-loot.tsx | Removes old loot update hook (moved to API layer). |
| apps/game-client/src/hooks/api/use-timers.tsx | Refactors timers query to centralized API and type export. |
| apps/game-client/src/hooks/api/use-timer-settings.tsx | Refactors timer settings queries/mutations to centralized API with logging. |
| apps/game-client/src/hooks/api/use-sound-settings.tsx | Refactors sound settings queries/mutations to centralized API with logging and updates cache shape. |
| apps/game-client/src/hooks/api/use-silent-cancel-party-gathering.ts | Adds logged-action wrapper and switches to centralized cancel API. |
| apps/game-client/src/hooks/api/use-silent-cancel-party-gathering.test.ts | Updates tests to mock centralized cancel API. |
| apps/game-client/src/hooks/api/use-send-chat-message.tsx | Refactors chat message sending to centralized API/types. |
| apps/game-client/src/hooks/api/use-search-npcs.tsx | Refactors NPC search to centralized API/types. |
| apps/game-client/src/hooks/api/use-reset-timer.tsx | Refactors reset timer mutation to centralized API/types. |
| apps/game-client/src/hooks/api/use-npcs.tsx | Refactors NPC fetching/types via centralized API, updates auth token usage. |
| apps/game-client/src/hooks/api/use-lootlog-character-config.tsx | Refactors lootlog config query/type via centralized API. |
| apps/game-client/src/hooks/api/use-guilds.tsx | Refactors guilds query/type via centralized API. |
| apps/game-client/src/hooks/api/use-guilds.test.tsx | Updates tests to mock fetchGuilds. |
| apps/game-client/src/hooks/api/use-guild.tsx | Refactors single guild query/type via centralized API. |
| apps/game-client/src/hooks/api/use-guild-permissions.ts | Refactors permissions query via centralized API. |
| apps/game-client/src/hooks/api/use-guild-members.tsx | Refactors members query + re-exports helpers from centralized API. |
| apps/game-client/src/hooks/api/use-guild-members.test.ts | Updates tests to mock getApiClient used by centralized API. |
| apps/game-client/src/hooks/api/use-delete-timer.tsx | Refactors delete timer mutation to centralized API/types. |
| apps/game-client/src/hooks/api/use-create-party-gathering.ts | Refactors party-gathering creation to centralized API/types. |
| apps/game-client/src/hooks/api/use-create-notification.tsx | Refactors notification creation to centralized API/types. |
| apps/game-client/src/hooks/api/use-create-manual-timer.tsx | Refactors manual timer creation to centralized API/types. |
| apps/game-client/src/hooks/api/use-create-loot.tsx | Removes old loot create hook (moved to API layer). |
| apps/game-client/src/hooks/api/use-chat-messages.tsx | Refactors chat messages query/types to centralized API. |
| apps/game-client/src/hooks/api/use-character-list.tsx | Refactors character list fetching to centralized API. |
| apps/game-client/src/hooks/api/use-cancel-party-gathering.ts | Adds logged-action metadata and switches to centralized cancel API. |
| apps/game-client/src/hooks/api/use-cancel-party-gathering.test.ts | Updates tests to mock centralized cancel API. |
| apps/game-client/src/hooks/api/use-api-client.tsx | Switches hooks to use shared getApiClient. |
| apps/game-client/src/hooks/api/cancel-party-gathering.ts | Re-exports cancel API/types from centralized module. |
| apps/game-client/src/helpers/mappers/battlelog.mappers.ts | Updates battle payload import to centralized API types. |
| apps/game-client/src/features/settings/settings.tsx | Translates settings window title via i18n. |
| apps/game-client/src/features/settings/constants/settings-tabs.ts | Adds canonical settings tab value list/type. |
| apps/game-client/src/features/settings/components/timers/timers-settings-tab.tsx | Migrates timers tab UI to shared layout + i18n + subtab styles. |
| apps/game-client/src/features/settings/components/timers/timers-settings-colors.tsx | Wraps timers colors list in settings section + i18n. |
| apps/game-client/src/features/settings/components/timers/timers-settings-appearance.tsx | Refactors appearance controls with shared control rows + i18n. |
| apps/game-client/src/features/settings/components/timers/components/hidden-colors-list.tsx | Translates hidden colors UI strings. |
| apps/game-client/src/features/settings/components/timers/components/default-color-item.tsx | Translates preview UI strings. |
| apps/game-client/src/features/settings/components/timers/components/custom-color-item.tsx | Translates preview UI strings. |
| apps/game-client/src/features/settings/components/timers/components/color-edit-form.tsx | Translates input placeholder and preview. |
| apps/game-client/src/features/settings/components/sounds/sound-field-input.tsx | Translates tooltip label. |
| apps/game-client/src/features/settings/components/sounds/master-volume-control.tsx | Uses SettingsPanel and translates labels/tooltips. |
| apps/game-client/src/features/settings/components/sounds/category-volume-control.tsx | Translates mute/unmute tooltip labels. |
| apps/game-client/src/features/settings/components/sounds/category-accordion-item.tsx | Translates disabled hint and formats note. |
| apps/game-client/src/features/settings/components/shared/settings-guild-selection-grid.test.tsx | Adds tests ensuring single vs multi select behavior. |
| apps/game-client/src/features/settings/components/notifications/notifications-settings-tab.tsx | Reworks notifications settings UX to per-type tabs + sync status indicator. |
| apps/game-client/src/features/settings/components/logs/logs.helpers.ts | Adds helper functions for log formatting/filtering/search. |
| apps/game-client/src/features/settings/components/logs/logs.constants.ts | Adds log filter values and i18n key maps. |
| apps/game-client/src/features/settings/components/logs/logs-entry-card.tsx | Adds placeholder logs entry card component. |
| apps/game-client/src/features/settings/components/logs/logs-api-request-card.tsx | Adds UI for rendering/copying logged API request details. |
| apps/game-client/src/features/settings/components/hidden-timers/hidden-timers.tsx | Uses settings panels/empty state + i18n and safer key handling. |
| apps/game-client/src/features/settings/components/hidden-timers/hidden-timers-tab.tsx | Refactors hidden timers tab to settings layout + guild grid selector + auto-selection logic. |
| apps/game-client/src/features/settings/components/hidden-timers/hidden-timers-tab.test.tsx | Adds tests for auto-select and grouping behavior. |
| apps/game-client/src/features/settings/components/general/general-settings-tab.tsx | Refactors general tab to shared layout/controls + i18n. |
| apps/game-client/src/features/settings/components/detector/detector-routing-settings-translations.ts | Adds translation getter for detector routing strings. |
| apps/game-client/src/features/settings/components/detector/detector-routing-guild-preview-tile.tsx | Adds guild preview tile component with tooltip. |
| apps/game-client/src/features/settings/components/battle-panel/battle-panel-settings-tab.tsx | Refactors battle panel tab to shared layout/controls + i18n. |
| apps/game-client/src/features/public-api/mappers.ts | Switches mapped types to centralized API exports. |
| apps/game-client/src/features/party-finder/hooks/use-party-gathering-socket.ts | Adds notification mutes + backend settings readiness gating and pending queue logic. |
| apps/game-client/src/features/party-finder/components/create-party-gathering-form.tsx | Updates response shape to centralized API result types. |
| apps/game-client/src/features/npc-detector/npc-detector.tsx | Uses backend-synced detector settings; adjusts NPC type resolution and window sizing. |
| apps/game-client/src/features/npc-detector/components/npcs-list.tsx | Adds animated list + scroll-to-top on detection cycles. |
| apps/game-client/src/features/notifications/utils/notification-mutes.test.ts | Adds tests for mute matching and key stability/deduplication. |
| apps/game-client/src/features/notifications/notifications.tsx | Makes notifications window open state controlled and adds fixed polling tick. |
| apps/game-client/src/features/notifications/hooks/use-notifications.tsx | Adds mute checks + backend settings readiness gating and pending queue logic; opens window on push. |
| apps/game-client/src/features/notifications/components/single-notification-party-gathering.tsx | Simplifies party gathering notification rendering; accepts meetsLevelReq externally. |
| apps/game-client/src/features/notifications/components/single-notification-npc.tsx | Simplifies NPC notification rendering; removes volunteer UI in-card. |
| apps/game-client/src/features/notifications/components/single-notification-message.tsx | Simplifies message notification rendering. |
| apps/game-client/src/features/notifications/components/notifications-list.tsx | Adds animated list + scroll-to-top on notification cycles. |
| apps/game-client/src/features/notifications/components/notification-mute-menu.tsx | Adds UI + mutation wiring to mute player/NPC notifications via user prefs. |
| apps/game-client/src/features/notifications/components/notification-mute-menu.test.tsx | Adds tests verifying disabled state + mutation payload uses fetched mutes. |
| apps/game-client/src/features/error-boundary/get-error-boundary-details.ts | Adds safe extraction/serialization utilities for error boundary UI. |
| apps/game-client/src/features/error-boundary/error-boundary.constants.ts | Centralizes error window sizing constants. |
| apps/game-client/src/features/error-boundary/error-boundary-translations.ts | Adds simple EN/PL error boundary strings. |
| apps/game-client/src/features/error-boundary/app-error-boundary-fallback.tsx | Adds draggable error fallback window with copy-to-clipboard. |
| apps/game-client/src/features/command/hooks/use-party-command.ts | Updates response shape to centralized API result types. |
| apps/game-client/src/features/chat/hooks/use-chat-messages.ts | Refactors listener to use centralized API and new cache shapes. |
| apps/game-client/src/features/chat/chat.tsx | Updates listener usage (no client param). |
| apps/game-client/src/features/catching-whitelist-warning/catching-whitelist-warning.tsx | Switches whitelist checks to unified catchingGuildIds. |
| apps/game-client/src/features/backend-preferences-warning/backend-preferences-warning.tsx | Adds new modal/window warning about backend preference migration. |
| apps/game-client/src/components/ui/select.tsx | Updates select styling and sizing classes. |
| apps/game-client/src/components/ui/scroll-area.tsx | Tweaks viewport child layout styling for consistent rendering. |
| apps/game-client/src/components/ui/collapsible.tsx | Adds trigger variants and optional chevron. |
| apps/game-client/src/components/ui/button.tsx | Adds button variants with centralized class mapping. |
| apps/game-client/src/components/settings/settings-tab-layout.tsx | Adds shared layout wrapper for settings tabs. |
| apps/game-client/src/components/settings/settings-sync-status.tsx | Adds UI indicator for idle/loading/saving/error. |
| apps/game-client/src/components/settings/settings-styles.ts | Adds shared class constants for settings subtabs. |
| apps/game-client/src/components/settings/settings-section.tsx | Adds section wrapper with header/description/actions. |
| apps/game-client/src/components/settings/settings-panel.tsx | Adds reusable panel styling for settings blocks. |
| apps/game-client/src/components/settings/settings-empty-state.tsx | Adds consistent empty state component for settings. |
| apps/game-client/src/components/settings/settings-control-row.tsx | Adds label/description/control row component. |
| apps/game-client/src/components/guild-switcher.tsx | Adds grid layout and multi-select support; supports custom button styling. |
| apps/game-client/src/components/guild-button.tsx | Adds aria-pressed and enhanced selected styling; supports custom className. |
| apps/game-client/src/components/delete-timer-popover.tsx | Refactors permission fetches to centralized API. |
| apps/game-client/src/api/users.api.ts | Adds centralized users API wrapper. |
| apps/game-client/src/api/settings.api.ts | Adds centralized settings API wrappers with logged actions. |
| apps/game-client/src/api/preferences.api.ts | Adds centralized preferences + lootlog config API wrappers with logged actions. |
| apps/game-client/src/api/npcs.api.ts | Adds centralized NPC API wrappers and types. |
| apps/game-client/src/api/loot.api.ts | Adds centralized loot API wrappers with logged actions and richer response types. |
| apps/game-client/src/api/index.ts | Aggregates API exports for client consumption. |
| apps/game-client/src/api/guilds.api.ts | Adds centralized guilds/members/permissions/worlds API wrappers and helpers. |
| apps/game-client/src/api/chat.api.ts | Adds centralized chat API wrappers with logged action aggregation. |
| apps/game-client/src/api/characters.api.ts | Adds centralized character list fetching logic. |
| apps/game-client/src/api/battle.api.ts | Adds centralized battle APIs with logged actions and type exports. |
| apps/game-client/src/api/auth.api.ts | Adds centralized auth token/scopes fetch functions. |
| apps/game-client/src/App.tsx | Switches to backend settings sync hook; adds error boundary fallback and backend prefs warning. |
| apps/game-client/package.json | Adds i18next/react-i18next dependencies. |
| apps/game-client/.gitignore | Alters log ignore rule (currently disables ignoring logs). |
| apps/api/test/loots.e2e-spec.ts | Updates whitelist payload shape and asserts richer loot response details. |
| apps/api/test/account-deletion.e2e-spec.ts | Updates whitelist payload shape to unified catching whitelist. |
| apps/api/src/users/users.controller.ts | Adds endpoints for game-account-scoped preferences (GET/PATCH). |
| apps/api/src/users/users.controller.spec.ts | Adds test ensuring controller delegates account preference reads. |
| apps/api/src/users/dto/update-user-preferences.dto.ts | Adds schema support for notification mutes in user preferences updates. |
| apps/api/src/users/dto/update-user-account-preferences.dto.ts | Adds DTO for updating game-account-scoped detector/notification preferences. |
| apps/api/src/user-lootlog-config/user-lootlog-config.service.ts | Unifies whitelist fields into catchingGuildIds and updates cleanup/upsert logic. |
| apps/api/src/user-lootlog-config/dto/create-user-account-config.dto.ts | Updates DTO to accept catchingGuildIds. |
| apps/api/src/timers/timers.service.spec.ts | Updates test module wiring for new Lootlog config dependency. |
| apps/api/src/timers/timers.controller.ts | Adds endpoint to create automatic timers from catching settings. |
| apps/api/src/timers/enum/error-key.enum.ts | Adds new timer error keys for empty whitelist/failed acceptance. |
| apps/api/src/timers/dto/create-auto-timer-response.dto.ts | Adds DTO describing submitted/rejected guild outcomes for auto timers. |
| apps/api/src/shared/dto/user-preferences-response.dto.ts | Extends user preferences response to include mutes. |
| apps/api/src/shared/dto/user-lootlog-config-response.dto.ts | Updates lootlog config response to unified catchingGuildIds. |
| apps/api/src/shared/dto/user-account-preferences-response.dto.ts | Adds DTO for account-scoped game preferences response. |
| apps/api/src/loots/loots.controller.spec.ts | Updates expected loot controller result shape to include guild outcome lists. |
| apps/api/src/loots/dto/loot-response.dto.ts | Extends loot create response DTO with submitted/rejected guild outcomes. |
| apps/api/src/kills/kills.service.ts | Switches targeting guild resolution to catchingGuildIds. |
| apps/api/src/kills/kills.service.spec.ts | Updates tests to use unified catching whitelist. |
| apps/api/prisma/schema.prisma | Renames whitelist columns and adds UserGameAccountSettings model. |
| apps/api/prisma/migrations/20260416113000_unify_catching_whitelist/migration.sql | Migrates two whitelist columns into catchingGuildIds and drops old columns. |
| apps/api/prisma/migrations/20260415111302_game_account_settings/migration.sql | Alters updatedAt default for game account settings table. |
| apps/api/prisma/migrations/20260413110000_user_game_account_settings/migration.sql | Creates UserGameAccountSettings table and migrates legacy JSON data if present. |
| AGENTS.md | Adds guideline to avoid splitting value/type imports; adds sandbox permission escalation note. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,5 @@ | |||
| # Logs | |||
| logs | |||
| # logs | |||
There was a problem hiding this comment.
Line 2 comments out the logs ignore rule, so a logs/ directory will no longer be ignored and may start being committed/packaged. If the intent is still to ignore runtime log directories, restore the logs entry (uncomment it) or replace with a more explicit pattern like logs/.
Suggested change
| # logs | |
| logs/ |
Comment on lines
+5
to
+6
| const MARGONEM_CHARTACTER_LIST_URL = | ||
| "https://public-api.margonem.pl/account/charlist"; |
There was a problem hiding this comment.
Typo in identifier: MARGONEM_CHARTACTER_LIST_URL should be MARGONEM_CHARACTER_LIST_URL for consistency and readability.
Comment on lines
+5
to
+11
| type SettingsSyncStatusProps = { | ||
| status: "idle" | "loading" | "saving" | "syncing" | "error"; | ||
| savingLabel: string; | ||
| syncingLabel: string; | ||
| errorLabel: string; | ||
| className?: string; | ||
| }; |
There was a problem hiding this comment.
The component's status union includes both loading and syncing, but the render logic only distinguishes saving vs "not saving" (using syncingLabel). In the current usage, loading will display the syncing label, and syncing seems unused. Consider either (a) collapsing statuses to idle | saving | syncing | error and mapping query-loading to syncing, or (b) adding a dedicated loadingLabel and explicitly rendering it when status === \"loading\".
Comment on lines
+52
to
+55
| const pendingNotificationsRef = useRef<Notification[]>([]); | ||
| const processNotificationRef = useRef<(data: Notification) => void>( | ||
| () => undefined, | ||
| ); |
There was a problem hiding this comment.
While settings/mutes are not ready, incoming socket notifications are appended to pendingNotificationsRef with no upper bound. If readiness is delayed (or fails), this list can grow indefinitely and increase memory usage. Consider adding a cap (drop oldest), or alternatively avoid queueing full payloads (store just IDs / last N), or flush with a timeout/fallback strategy.
Comment on lines
+101
to
105
| const handler = (data: Notification) => { | ||
| if (!isReadyRef.current || !areMutesReadyRef.current) { | ||
| pendingNotificationsRef.current.push(data); | ||
| return; | ||
| } |
There was a problem hiding this comment.
While settings/mutes are not ready, incoming socket notifications are appended to pendingNotificationsRef with no upper bound. If readiness is delayed (or fails), this list can grow indefinitely and increase memory usage. Consider adding a cap (drop oldest), or alternatively avoid queueing full payloads (store just IDs / last N), or flush with a timeout/fallback strategy.
Comment on lines
23
to
36
| if (!useChatCache.getState().messageCache[data.guildId]) { | ||
| fetchChatMessages(client, data.guildId).then((messages) => { | ||
| fetchChatMessages(data.guildId).then((messages) => { | ||
| if (messages.length) { | ||
| useChatCache.getState().setMessageCache(data.guildId, messages); | ||
| } | ||
| }); | ||
| } | ||
| if (!useChatCache.getState().memberCache[data.guildId]) { | ||
| fetchGuildMembers(client, data.guildId).then((members) => { | ||
| fetchGuildMembers(data.guildId).then((members) => { | ||
| if (members) { | ||
| useChatCache.getState().setMemberCache(data.guildId, members); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
These background fetches are started without a .catch(...). If either request rejects, it can surface as an unhandled promise rejection in the console/runtime. Add error handling (even a catch(() => undefined) or a logged warning) to prevent unhandled rejections.
Comment on lines
+25
to
+36
| export const useUpdateUserPreferences = () => { | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| return useMutation({ | ||
| mutationKey: getUpdateUserPreferencesMutationKey(), | ||
| mutationFn: (payload: UpdateUserPreferencesPayload) => | ||
| updateUserPreferences(payload), | ||
| onMutate: async (payload) => { | ||
| const queryKey = getUserPreferencesQueryKey(); | ||
| await queryClient.cancelQueries({ queryKey }); | ||
|
|
||
| const previousData = queryClient.getQueryData<UserPreferences>(queryKey); |
There was a problem hiding this comment.
The optimistic update/rollback behavior in useUpdateUserPreferences is non-trivial (partial mutes patching + rollback on error). Add unit tests covering: (1) optimistic merge when only mutes.players is provided, (2) optimistic merge when only mutes.npcs is provided, (3) rollback restores previous cache on error, and (4) onSuccess replaces cache with server response.
![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: 38ddb75805
ℹ️ 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 seededNotifications = data.hasStoredNotifications | ||
| ? data.notifications | ||
| : createNotificationsSettings(guildIds); |
There was a problem hiding this comment.
This seeding path runs as soon as guilds is a defined array, including the transient [] state, so createNotificationsSettings(guildIds) can persist empty guildIds and mark hasStoredNotifications true. After that, later guild data won’t trigger reseeding, and notification filters (guildIds.includes(...)) will suppress all notifications until the user manually reconfigures settings. This is reproducible when preferences are first seeded before guild memberships finish loading.
Useful? React with 👍 / 👎.
Comment on lines
94
to
+96
| useEffect(() => { | ||
| if (socket?.hasListeners(GatewayEvent.NOTIFICATION) || !connected) return; | ||
| pendingNotificationsRef.current = []; | ||
| }, [accountId]); |
There was a problem hiding this comment.
The queue is unconditionally cleared on every accountId change, so notifications received while settings/mutes are not ready are dropped during the common startup transition from null to a real account ID. Because the draining effect only runs after readiness, this reset can permanently lose in-flight notifications instead of replaying them once preferences load.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 179 out of 286 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+65
to
+72
| <SingleNotification | ||
| notification={notification} | ||
| notificationAnimationCycle={ | ||
| activeNotificationAnimations[notification.listKey] ?? null | ||
| } | ||
| guildNamesById={guildNamesById} | ||
| showCloseButton={notifications.length > 1} | ||
| /> |
There was a problem hiding this comment.
notifications is optional in props, but showCloseButton={notifications.length > 1} assumes it's always defined. This can cause a TypeScript error (and is fragile to refactors). Consider computing const notificationsCount = notifications?.length ?? 0; and using that instead.
| > | ||
| <ScrollAreaPrimitive.Viewport | ||
| className="ll:h-full ll:w-full ll:max-h-[inherit] ll:rounded-[inherit] ll:select-text" | ||
| className="ll:h-full ll:w-full ll:max-h-[inherit] ll:rounded-[inherit] ll:select-text ll:[&>div:first-child]:block!" |
There was a problem hiding this comment.
The Tailwind !important modifier syntax looks incorrect here (block!). Tailwind typically uses !block (or ...:!block depending on variant ordering). As written, this class may be ignored, leaving the layout issue unresolved.
Suggested change
| className="ll:h-full ll:w-full ll:max-h-[inherit] ll:rounded-[inherit] ll:select-text ll:[&>div:first-child]:block!" | |
| className="ll:h-full ll:w-full ll:max-h-[inherit] ll:rounded-[inherit] ll:select-text ll:[&>div:first-child]:!block" |
Comment on lines
9
to
14
| return useQuery({ | ||
| queryKey: ["auth-token"], | ||
| enabled: Boolean(sessionToken), | ||
| select: (data: { token: string }) => data.token, | ||
| queryFn: async () => { | ||
| const response = await fetch(`${AUTH_SERVICE_URL}/idp/token`, { | ||
| headers: { | ||
| Authorization: `Bearer ${sessionToken}`, | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error("Failed to fetch auth token"); | ||
| } | ||
|
|
||
| return response.json(); | ||
| }, | ||
| queryFn: () => fetchAuthToken(sessionToken as string), | ||
| refetchOnMount: true, | ||
| }); |
There was a problem hiding this comment.
This query result depends on sessionToken, but the queryKey does not. If the session token changes, React Query may serve a cached token from a previous session. Include sessionToken in the queryKey (e.g., ['auth-token', sessionToken]) to keep caching correct.
| const { data: token } = useAuthToken(); | ||
|
|
||
| const query = useQuery({ | ||
| queryKey: ["@me"], |
There was a problem hiding this comment.
Similarly, this query depends on token but the queryKey doesn't. If tokens can change (re-login, refresh, etc.), include the token (or a stable user/session identifier) in queryKey to avoid returning a cached user for a different token.
Suggested change
| queryKey: ["@me"], | |
| queryKey: ["@me", token], |
Comment on lines
+42
to
+49
| const nextMutes = payload.mutes | ||
| ? { | ||
| players: payload.mutes.players | ||
| ? payload.mutes.players.map((player) => ({ ...player })) | ||
| : cloneNotificationMutes(previousData.mutes).players, | ||
| npcs: payload.mutes.npcs | ||
| ? payload.mutes.npcs.map((npc) => ({ ...npc })) | ||
| : cloneNotificationMutes(previousData.mutes).npcs, |
There was a problem hiding this comment.
cloneNotificationMutes(previousData.mutes) is invoked twice in the fallback branches, which does extra work and can allocate more than needed. Consider cloning once (e.g., const previousMutes = cloneNotificationMutes(...)) and then reusing previousMutes.players/npcs.
Suggested change
| const nextMutes = payload.mutes | |
| ? { | |
| players: payload.mutes.players | |
| ? payload.mutes.players.map((player) => ({ ...player })) | |
| : cloneNotificationMutes(previousData.mutes).players, | |
| npcs: payload.mutes.npcs | |
| ? payload.mutes.npcs.map((npc) => ({ ...npc })) | |
| : cloneNotificationMutes(previousData.mutes).npcs, | |
| const previousMutes = cloneNotificationMutes(previousData.mutes); | |
| const nextMutes = payload.mutes | |
| ? { | |
| players: payload.mutes.players | |
| ? payload.mutes.players.map((player) => ({ ...player })) | |
| : previousMutes.players, | |
| npcs: payload.mutes.npcs | |
| ? payload.mutes.npcs.map((npc) => ({ ...npc })) | |
| : previousMutes.npcs, |
Comment on lines
4
to
9
| type UseWorldOptions = { | ||
| guildId?: string; | ||
| retry?: boolean; | ||
| }; | ||
|
|
||
| export const useWorlds = ({ guildId }: UseWorldOptions) => { |
There was a problem hiding this comment.
UseWorldOptions defines retry?: boolean, but the function never reads it (it only destructures guildId). This is misleading for callers. Either remove the option or wire it into useQuery({ retry }).
| import type { LoggedAction, SerializableValue } from "@/store/logs.store"; | ||
|
|
||
| export const stringifyLogValue = (value: SerializableValue): string => { | ||
| return JSON.stringify(value, null, 2); |
There was a problem hiding this comment.
JSON.stringify can produce undefined at runtime for some inputs (e.g., undefined, functions), which then propagates into UI/search paths. Consider normalizing this to a string (e.g., JSON.stringify(...) ?? '') so downstream consumers always receive a proper string.
Suggested change
| return JSON.stringify(value, null, 2); | |
| return JSON.stringify(value, null, 2) ?? ""; |
Comment on lines
+6
to
+14
| export const useCurrentUserNotificationMutes = () => { | ||
| const gameInitialized = useGlobalStore( | ||
| (state) => state.gameState.gameInitialized, | ||
| ); | ||
| const query = useUserPreferences(gameInitialized); | ||
| const mutes = useMemo( | ||
| () => getEffectiveNotificationMutes(query.data), | ||
| [query.data], | ||
| ); |
There was a problem hiding this comment.
New behavior for deriving mutes and exposing isReady is introduced here, but there’s no test covering the hook’s returned shape (e.g., defaults when query.data is undefined and isReady behavior). Since this repo already uses Vitest hook/component tests, adding a small use-current-user-notification-mutes.test.tsx would help prevent regressions.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 177 out of 288 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
apps/game-client/src/features/notifications/hooks/use-notifications.tsx:32
- The codebase elsewhere now calls getNpcTypeByWt with additional NPC fields (e.g., prof/type) to disambiguate types. Here it's still called with only wt, which can lead to incorrect notification category selection (and therefore wrong preferences/mute handling). Pass the available NPC fields (e.g., n.npc.prof and n.npc.type) to match the updated usage.
const getNotificationType = (n: Notification) => {
if (!n.npc || !n.npc.wt) return "message" as const;
return getNpcTypeByWt(NpcType, n.npc.wt);
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+13
to
20
| const { data: token } = useAuthToken(); | ||
| const guildId = undefined; // TODO: Add proper guild context | ||
|
|
||
| const queryParams = { | ||
| search: search ?? "", | ||
| }; | ||
|
|
||
| const query = useQuery({ | ||
| queryKey: ["guild-npcs", guildId, search], | ||
| queryFn: () => | ||
| axios.get<Npc[]>( | ||
| `${API_URL}/npcs?${new URLSearchParams(queryParams).toString()}`, | ||
| { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| }, | ||
| ), | ||
| queryFn: () => fetchNpcs(token as string, search), | ||
| enabled: !!token && !!guildId, | ||
| select: (response) => response.data, | ||
| }); |
There was a problem hiding this comment.
This hook currently can never fetch because "guildId" is hardcoded to undefined and the query is disabled when guildId is falsy. If the /npcs endpoint is not guild-scoped, remove guildId from the queryKey and enabled condition; otherwise, source guildId from the actual guild context/store and pass it here.
Comment on lines
+18
to
31
| export const Button: FC<ButtonProps> = ({ | ||
| className, | ||
| children, | ||
| variant = "default", | ||
| ...props | ||
| }) => { | ||
| return ( | ||
| <button | ||
| className={cn( | ||
| "ll:text-[12px] ll:border ll:border-gray-400 ll:bg-gray-400/30 ll:hover:bg-gray-400/50 ll:rounded-sm ll:h-5 ll:text-white ll:disabled:bg-gray-700/30 ll:disabled:text-gray-500 ll:disabled:cursor-not-allowed ll:transition-colors ll:flex ll:items-center ll:justify-center", | ||
| "ll:text-[12px] ll:border ll:rounded-sm ll:h-5 ll:disabled:bg-gray-700/30 ll:disabled:text-gray-500 ll:disabled:cursor-not-allowed ll:transition-colors ll:flex ll:items-center ll:justify-center", | ||
| BUTTON_VARIANT_CLASS_NAMES[variant], | ||
| className, | ||
| "ll-custom-cursor-pointer", | ||
| )} |
There was a problem hiding this comment.
Because this component does not set a default "type", HTML will default to type="submit" when used inside forms, which can cause accidental submissions. Consider defaulting to type="button" unless the caller explicitly provides a type via props.
| <AnimatedWindow isOpen={open} windowKey="backend-preferences-warning"> | ||
| <DraggableWindow | ||
| id="backend-preferences-warning" | ||
| title="Zmiana ustawień" |
There was a problem hiding this comment.
This new UI introduces more hard-coded Polish strings while other settings UI is being migrated to i18n (react-i18next). Consider switching this component to use translation keys (including the window title and button labels) to keep localization consistent across the app.
Comment on lines
+69
to
+77
| <div className="ll:text-sm ll:text-gray-200"> | ||
| <p className="ll:mb-3"> | ||
| Ustawienia powiadomień i wykrywacza są teraz przechowywane tylko | ||
| po stronie serwera. | ||
| </p> | ||
| <p> | ||
| Poprzednie ustawienia zapisane lokalnie w tej przeglądarce nie | ||
| zostały zachowane. Sprawdź aktualną konfigurację w ustawieniach. | ||
| </p> |
There was a problem hiding this comment.
This new UI introduces more hard-coded Polish strings while other settings UI is being migrated to i18n (react-i18next). Consider switching this component to use translation keys (including the window title and button labels) to keep localization consistent across the app.
| isGatheringParty?: boolean; | ||
| }; | ||
|
|
||
| const MAX_PENDING_NOTIFICATIONS = 100; |
There was a problem hiding this comment.
The new pending-notification buffering logic (including the MAX_PENDING_NOTIFICATIONS cap and the transition from "not ready" to "ready" flushing) is complex enough to merit dedicated tests. Consider adding tests that (1) verify the buffer caps at 100 and drops the oldest, and (2) verify buffered notifications flush once both settings and mutes become ready.
Comment on lines
+111
to
121
| const handler = (data: Notification) => { | ||
| if (!isReadyRef.current || !areMutesReadyRef.current) { | ||
| if ( | ||
| pendingNotificationsRef.current.length >= MAX_PENDING_NOTIFICATIONS | ||
| ) { | ||
| pendingNotificationsRef.current.shift(); | ||
| } | ||
|
|
||
| if (!typeSettings) { | ||
| pushNotification({ ...data, servers: [data.guildId] }); | ||
| pendingNotificationsRef.current.push(data); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The new pending-notification buffering logic (including the MAX_PENDING_NOTIFICATIONS cap and the transition from "not ready" to "ready" flushing) is complex enough to merit dedicated tests. Consider adding tests that (1) verify the buffer caps at 100 and drops the oldest, and (2) verify buffered notifications flush once both settings and mutes become ready.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lootlog-docs | ed37be9 | Commit Preview URL Branch Preview URL |
Apr 17 2026, 08:01 AM |
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.
No description provided.