feat: server-side owner filtering for WebSocket channels#133
feat: server-side owner filtering for WebSocket channels#133
Conversation
Add consumables channel support and optional per-subscription filters to the WebSocket subscription hub. Clients can now subscribe with an owner filter for summit and consumables channels to receive only updates relevant to their wallet address. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🚅 Deployed to the summit-pr-133 environment in Summit stack
1 service not affected by this PR
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the real-time data delivery system by introducing a dedicated WebSocket channel for consumable item updates and implementing server-side filtering capabilities. This allows for more targeted and efficient data distribution, ensuring clients only receive information relevant to their specific interests, such as a particular owner's assets. The changes improve network efficiency and user experience while maintaining compatibility with existing client implementations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @loothero's task —— View job Claude Review - General Engineering ReviewTodo List
run=22854132982 attempt=1 sha=4c8fa1ed5b6a60824eb419843cc7651498084530 scope=general Review FindingsAfter analyzing the PR diff with focus on files outside the main app packages (contracts/, client/, indexer/, api/), I found that this PR contains no changes to:
All changes in this PR are contained within the No issues found. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
|
Claude finished @loothero's task —— View job Claude Review - Indexer/API Reviewrun=22854132982 attempt=1 sha=4c8fa1ed5b6a60824eb419843cc7651498084530 scope=indexer-api Todo List
Findings[MEDIUM] api/src/ws/subscriptions.ts:175-176 - Missing PostgreSQL notification source validation [LOW] api/src/ws/subscriptions.ts:188-190 - Potential null owner handling inconsistency [LOW] api/src/ws/subscriptions.ts:286-292 - Filter mutation risk during subscription [INFO] api/src/ws/subscriptions.ts:321 - Subscription response includes echoed filters Analysis SummaryThe WebSocket filtering implementation is generally well-structured and follows good practices:
The main concern is the missing verification that the Summary: 0 CRITICAL, 0 HIGH, 1 MEDIUM, 2 LOW, 1 INFO |
Codex Review - General Engineering ReviewNo issues found. |
|
Claude finished @loothero's task —— View job Claude Review - React/Frontend Review
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Codex Review - Indexer/API Review[HIGH] api/src/ws/subscriptions.ts:122 - [HIGH] api/src/ws/subscriptions.ts:200 - Owner filter check is skipped when payload owner is [MEDIUM] api/src/ws/subscriptions.ts:289 - Filter owner normalization is lowercase-only, not canonical Starknet normalization ( [LOW] api/src/ws/subscriptions.ts:286 - Re-subscribing to an already subscribed channel without a filter does not clear a previously stored filter. Summary: 0 CRITICAL, 2 HIGH, 1 MEDIUM, 1 LOW, 0 INFO |
![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: 4c8fa1ed5b
ℹ️ 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".
| const wsFilters: Partial<Record<"summit" | "event" | "consumables", ChannelFilter>> | undefined = | ||
| account?.address | ||
| ? { | ||
| summit: { owner: addAddressPadding(account.address) }, |
There was a problem hiding this comment.
Applying summit: { owner: ... } here causes connected players to stop receiving summit updates whenever the current summit beast is owned by someone else, because the API now filters by owner before broadcasting. GameDirector depends on those global summit updates to keep the live summit state fresh (it only fetches on mount), so most connected users will see stale summit health/holder data until a reload.
Useful? React with 👍 / 👎.
| if (filters && Object.keys(filters).length > 0) { | ||
| subscribeMsg.filters = filters; | ||
| } |
There was a problem hiding this comment.
Filters are only attached to the subscribe payload during ws.onopen, so if the caller changes filters later (for example wallet connect/disconnect or account switch), the server-side subscription remains stale for the lifetime of that socket. In this PR that means owner-scoped streams can keep targeting the previous address and the new account can miss consumables updates until a reconnect happens.
Useful? React with 👍 / 👎.
Codex Review - React/Frontend Review[HIGH] client/src/contexts/GameDirector.tsx:437 - [MEDIUM] client/src/hooks/useWebSocket.ts:252 - WebSocket lifecycle effect ignores [LOW] client/src/contexts/GameDirector.tsx:445 - Summary: 0 CRITICAL, 1 HIGH, 1 MEDIUM, 1 LOW, 0 INFO |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces server-side filtering for WebSocket channels, a valuable feature for reducing client-side processing and network traffic. The implementation is well-done, adding a new consumables channel and filters to subscriptions, with comprehensive updates to documentation and tests on both the client and server. I've identified one potential issue in the server-side filtering logic that could cause messages with no owner to be incorrectly sent to filtered clients. Once that is addressed, this will be a solid addition.
|
|
||
| // Apply owner filter for summit and consumables channels | ||
| const filter = client.filters.get(channel); | ||
| if (filter?.owner && payloadOwner !== null && filter.owner !== payloadOwner) { |
There was a problem hiding this comment.
The current filtering logic may not behave as expected for payloads without an owner. If a client subscribes with an owner filter, they will still receive messages for the summit and consumables channels if the message payload has a null or undefined owner. This is because the payloadOwner !== null check short-circuits the filter logic.
A client filtering by a specific owner should likely only receive messages that match that owner, and messages without an owner should be skipped. You can fix this by removing the payloadOwner !== null check.
It would also be beneficial to add a test case for this scenario (a client with an owner filter receiving a message with a null owner) to prevent regressions.
| if (filter?.owner && payloadOwner !== null && filter.owner !== payloadOwner) { | |
| if (filter?.owner && filter.owner !== payloadOwner) { |
There was a problem hiding this comment.
Pull request overview
This PR adds a new consumables WebSocket channel for real-time potion balance updates and introduces server-side owner-based filtering for WebSocket subscriptions. The API's SubscriptionHub listens on the new consumables_update PG NOTIFY channel and applies per-client owner filters during broadcast. The client is updated to subscribe to the new channel, pass wallet-address filters, and process consumable balance updates in GameDirector.
Changes:
- Added
consumableschannel withConsumablesPayloadtype, PG LISTEN, and broadcast support in the API'sSubscriptionHub, along with aChannelFiltermechanism that allows per-subscription owner filtering onsummitandconsumableschannels - Updated the client
useWebSockethook to supportfiltersandonConsumables, and wiredGameDirectorto pass owner filters and handle consumable balance updates with a client-side safety fallback - Added comprehensive tests: 13 new server-side tests and 9 new client-side tests covering channel subscription, filter behavior, and normalization
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
api/src/ws/subscriptions.ts |
Core: ChannelFilter, filters map per client, owner-aware broadcast, LISTEN on consumables_update |
api/src/ws/subscriptions.test.ts |
13 new tests for consumables channel and filter behavior |
api/src/index.ts |
Root endpoint docs updated with new channel and filter example |
api/AGENTS.md |
WebSocket documentation updated with consumables channel and filter info |
client/src/hooks/useWebSocket.ts |
Hook updated with ConsumablesData, ChannelFilter types, filter serialization |
client/src/hooks/useWebSocket.test.ts |
New test file with 5 tests for consumables and filter serialization |
client/src/contexts/GameDirector.tsx |
handleConsumables handler, wsFilters with padded owner address |
client/src/contexts/GameDirector.test.tsx |
4 new tests for consumables handling and filter passing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const wsFilters: Partial<Record<"summit" | "event" | "consumables", ChannelFilter>> | undefined = | ||
| account?.address | ||
| ? { | ||
| summit: { owner: addAddressPadding(account.address) }, | ||
| consumables: { owner: addAddressPadding(account.address) }, | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
Bug: Applying an owner filter on the summit channel will cause the client to only receive summit updates when the connected wallet's beast is on the summit. The summit is a shared resource — all clients need to see every summit update to keep the game state current (e.g., tracking when the summit beast changes, marking previous beasts as dead). The wsFilters object should not include a filter for summit; only consumables should have an owner filter.
| updateConnectionState("disconnected"); | ||
| } | ||
| }, [url, enabled, channels, handleMessage, updateConnectionState]); | ||
| }, [url, enabled, channels, filters, handleMessage, updateConnectionState]); |
There was a problem hiding this comment.
The filters object is included in the connect dependency array, but the useEffect that triggers connection (line 243-252) only depends on [url, enabled]. This means if the user connects their wallet after the WebSocket is already open, the new filters won't be sent to the server until the next natural reconnection (e.g., network drop). The client-side fallback check in handleConsumables mitigates this, but the server-side filtering won't be active until reconnection.
Consider either: (1) storing filters in a ref (like callbacks) to avoid recreating connect, or (2) adding logic to send a re-subscribe message when filters change on an already-open connection, rather than requiring a full reconnection.
| const filters = data.filters || {}; | ||
| this.subscribe(id, subChannels, filters); | ||
| if (!this.send(id, client.ws, JSON.stringify({ type: "subscribed", channels: subChannels, filters }))) { |
There was a problem hiding this comment.
The subscribed confirmation echoes back the raw filters object from the client (data.filters) rather than the normalized version stored internally (where owner is lowercased). This means the response may show "owner": "0xAAA" while the server actually stores and uses "owner": "0xaaa". Consider echoing the normalized filters from the subscription state, or at minimum document that the echoed filters may differ from the stored ones.
Summary
consumableschannel to the WebSocket subscription hub (PG LISTEN onconsumables_update)filtersparameter to subscribe messages, enabling per-subscription server-side filteringsummitandconsumableschannels supportownerfilter to receive only updates matching a specific addresseventchannel remains unfiltered (global activity feed)addAddressPaddinghandleConsumablesas a safety fallbackChanged files
API:
api/src/ws/subscriptions.ts--ChannelFiltertype, filters stored per client per channel, broadcast checks owner matchapi/src/ws/subscriptions.test.ts-- 13 new tests (consumables channel + channel filter behavior)api/src/index.ts-- root endpoint docs updated with filter exampleapi/AGENTS.md-- WebSocket docs updatedClient:
client/src/hooks/useWebSocket.ts--ConsumablesData,ChannelFiltertypes,filtersoption,onConsumablescallbackclient/src/hooks/useWebSocket.test.ts-- new test file (5 tests including filter serialization)client/src/contexts/GameDirector.tsx--handleConsumableshandler,wsFilterswith padded owner addressclient/src/contexts/GameDirector.test.tsx-- 4 new tests (consumables handling + filter passing)Test plan
cd api && pnpm exec tsc --noEmit-- passescd api && pnpm test-- 92 tests pass (34 subscription tests including 13 new)cd client && pnpm lint-- passescd client && pnpm exec tsc -b-- passescd client && pnpm test-- 192 tests pass (including 9 new)Generated with Claude Code