Skip to content

Add channel storage page#436

Merged
tudddorrr merged 1 commit intodevelopfrom
channel-storage-view
Dec 31, 2025
Merged

Add channel storage page#436
tudddorrr merged 1 commit intodevelopfrom
channel-storage-view

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Dec 31, 2025
@claude
Copy link

claude bot commented Dec 31, 2025

PR Review: Add channel storage page

🟡 Code Quality and Best Practices

Issue 1: Inconsistent SWR key construction in useChannelStorage.ts:23

The SWR key includes both search and page separately, but the fetcher function only receives the URL. This means the fetcher doesn't actually use the updated search and page values from the SWR key.

// Current (incorrect)
const { data, error } = useSWR(
  [`/games/${activeGame.id}/game-channels/${channelId}/storage`, search, page],
  fetcher
)

Compare with useLeaderboardEntries.ts:40-44 which correctly includes all dependencies in both the key AND fetcher params.

Fix: The fetcher should receive all dependency values from the SWR key, or the search/page should be accessed from the closure (current approach works but is inconsistent with codebase patterns).


🟡 Potential Bugs or Issues

Issue 2: Missing null check for channelId in ChannelStorage.tsx:36

Using the non-null assertion operator (channelId!) and Number() without validation could cause issues if the route param is missing or invalid.

// Current (line 36)
useChannelStorage(activeGame, Number(channelId!), debouncedSearch, page)

While the 404 handling exists, Number(undefined) returns NaN which would make an invalid API request before the 404 is caught. Other pages like PlayerEvents.tsx:29 use the same pattern but pair it with validation hooks.

Fix: Add validation or use a default value to avoid NaN.


🔵 Performance Considerations

Issue 3: Empty dependency in Channels.tsx useEffect

Line 43-45 has a useEffect with editingChannel in the dependency array but it's never used in the effect body:

useEffect(() => {
  if (!showModal) setEditingChannel(null)
}, [showModal, editingChannel])  // editingChannel is unnecessary

This causes the effect to run on every editingChannel change even though it only needs showModal.

Fix: Remove editingChannel from the dependency array.


Security Concerns

No issues found


Overall: The PR follows the project patterns well. The main concerns are consistency with existing SWR hook patterns and a minor redundant dependency in useEffect. The functionality looks solid otherwise.

@tudddorrr tudddorrr force-pushed the channel-storage-view branch 2 times, most recently from 21e3b1d to 93e079b Compare December 31, 2025 12:41
@tudddorrr tudddorrr force-pushed the channel-storage-view branch from 93e079b to 818b364 Compare December 31, 2025 12:46
@tudddorrr tudddorrr enabled auto-merge December 31, 2025 12:46
@tudddorrr tudddorrr merged commit d0e63bf into develop Dec 31, 2025
5 checks passed
@tudddorrr tudddorrr deleted the channel-storage-view branch December 31, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant