docs: normalize navbar logos & refresh README#349
Open
deekshi275 wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR condenses README content into a concise project overview, standardizes favicon/navbar logo references to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SocketServer
participant IPTracker
participant RateLimiter
participant ConnectedUsers
Client->>SocketServer: socket.connect()
SocketServer->>IPTracker: getClientIP(socket)
SocketServer->>SocketServer: checkConnectionLimits(socket)
alt exceeds limits
SocketServer-->>Client: disconnect / error (limits exceeded)
else accepted
SocketServer->>IPTracker: increment ipConnectionCounts
SocketServer->>ConnectedUsers: register activeConnections
Client->>SocketServer: emit "join" {wallet,...}
SocketServer->>RateLimiter: checkRateLimit(socket,"join")
alt rate-limited
SocketServer-->>Client: emit "error" (rate limit)
else allowed
SocketServer->>SocketServer: validate wallet address
alt invalid wallet
SocketServer-->>Client: emit "error" (invalid wallet)
else valid
SocketServer->>ConnectedUsers: attach socket to user
SocketServer-->>Client: emit "joined"
end
end
end
Client->>SocketServer: disconnect
SocketServer->>ConnectedUsers: remove socket
SocketServer->>IPTracker: decrement ipConnectionCounts
SocketServer->>RateLimiter: cleanup per-socket rate state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
public/dashboard.html (1)
16-18: Minor inconsistency:apple-touch-iconstill referencesfavicon.png.Line 18 uses
favicon.pngforapple-touch-iconwhile the rest of the favicon/logo references now uselogo-32x32.png. If the goal is full logo normalization, consider updating this as well—or keep it if a separate higher-resolution asset is intentionally used for iOS.Proposed fix if normalization is intended
- <link rel="apple-touch-icon" href="favicon.png"> + <link rel="apple-touch-icon" href="logo-32x32.png">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/dashboard.html` around lines 16 - 18, The apple-touch-icon link currently points to "favicon.png" while the other favicon links use "logo-32x32.png"; update the <link rel="apple-touch-icon" href="favicon.png"> entry to reference the normalized asset ("logo-32x32.png") if you intend to standardize icons, or replace it with a higher-resolution iOS-specific asset and filename if you intentionally want a different image—adjust the href accordingly so <link rel="apple-touch-icon"> matches the chosen branding asset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 123: The README contains invalid characters in the date string
"March?5?2026"; replace the non-standard characters with standard ASCII so it
reads "March 5 2026" (or "March 5, 2026" if you prefer a comma), ensuring you
remove any non-breaking spaces or encoding artifacts; locate and update the
exact string "March?5?2026" in the README to use plain spaces and ASCII
punctuation.
- Around line 74-77: Update the README clone instructions so the post-clone
directory matches the repository name: change the cd target referenced in the
snippet from "blockchain-evidence-1" to "blockchain-evidence" (the directory
created by git clone https://github.com/Gooichand/blockchain-evidence.git) to
avoid a failing cd command; update the snippet where the clone and cd commands
appear.
- Around line 93-104: The README's fenced code block listing the repo tree (the
block starting with "blockchain-evidence/") is missing a language specifier;
update the opening fence to include a language such as `text` or `plaintext`
(e.g., change ``` to ```text) so the code block renders consistently across
static analysis and markdown renderers.
- Line 119: The README now states MIT but the repository still contains an
Apache-2.0 LICENSE and package.json lists "license": "Apache-2.0"; fix the
inconsistency by choosing the intended license and updating all three places
consistently: either revert the README text to reference Apache 2.0, or replace
the LICENSE file contents with the MIT license text and update package.json's
"license" field to "MIT" (and update any license header comments if present).
Ensure the README license line, the LICENSE file contents, and the package.json
"license" property all match exactly.
- Around line 19-26: The Table of Contents links (e.g., link target "Features")
don't match the actual section headers that include emojis (e.g., "✨ Features",
"📦 Tech Stack", "🚀 Project Phases", "⚙️ Getting Started", "📁 Folder
Structure", "🤝 Contributing", "📜 License"), so anchors won't navigate; fix by
either removing the emoji prefixes from those headers so their slug becomes
simple (e.g., "Features") OR update each TOC entry to the exact anchor slug
produced by your renderer (for GitHub-style slugs include the emoji and hyphen
pattern, e.g., the slug for "✨ Features" becomes the renderer-specific form such
as "✨-features" or "-features"); update the TOC lines (1–7) to match the chosen
header form and verify links work in the README viewer.
---
Nitpick comments:
In `@public/dashboard.html`:
- Around line 16-18: The apple-touch-icon link currently points to "favicon.png"
while the other favicon links use "logo-32x32.png"; update the <link
rel="apple-touch-icon" href="favicon.png"> entry to reference the normalized
asset ("logo-32x32.png") if you intend to standardize icons, or replace it with
a higher-resolution iOS-specific asset and filename if you intentionally want a
different image—adjust the href accordingly so <link rel="apple-touch-icon">
matches the chosen branding asset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c1c8c15-421c-4d70-967f-b00c2c98da81
📒 Files selected for processing (3)
README.mdpublic/cases.htmlpublic/dashboard.html
Comment on lines
+19
to
+26
| ## 📗 Table of Contents | ||
| 1. [Features](#features) | ||
| 2. [Tech Stack](#tech-stack) | ||
| 3. [Project Phases](#project-phases) | ||
| 4. [Getting Started](#getting-started) | ||
| 5. [Folder Structure](#folder-structure) | ||
| 6. [Contributing](#contributing) | ||
| 7. [License](#license) |
Contributor
There was a problem hiding this comment.
Table of Contents links won't navigate correctly due to emoji prefixes in headers.
The anchor fragments (e.g., #features) don't match the actual header slugs which include emojis (e.g., ## ✨ Features generates a slug like ✨-features or -features depending on the Markdown renderer). Static analysis flagged these as invalid fragments.
Proposed fix: Either remove emojis from headers or update anchors
Option 1 – Remove emojis from section headers (recommended for cleaner anchors):
-## ✨ Features
+## Features
-## 🛠 Tech Stack
+## Tech Stack(and similarly for other headers)
Option 2 – Keep emojis but update anchor links (GitHub-specific slugs):
-1. [Features](`#features`)
-2. [Tech Stack](`#tech-stack`)
-3. [Project Phases](`#project-phases`)
-4. [Getting Started](`#getting-started`)
-5. [Folder Structure](`#folder-structure`)
-6. [Contributing](`#contributing`)
-7. [License](`#license`)
+1. [Features](`#-features`)
+2. [Tech Stack](`#-tech-stack`)
+3. [Project Phases](`#-project-phases`)
+4. [Getting Started](`#-getting-started`)
+5. [Folder Structure](`#-folder-structure`)
+6. [Contributing](`#-contributing`)
+7. [License](`#-license`)📝 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
| ## 📗 Table of Contents | |
| 1. [Features](#features) | |
| 2. [Tech Stack](#tech-stack) | |
| 3. [Project Phases](#project-phases) | |
| 4. [Getting Started](#getting-started) | |
| 5. [Folder Structure](#folder-structure) | |
| 6. [Contributing](#contributing) | |
| 7. [License](#license) | |
| ## 📗 Table of Contents | |
| 1. [Features](`#-features`) | |
| 2. [Tech Stack](`#-tech-stack`) | |
| 3. [Project Phases](`#-project-phases`) | |
| 4. [Getting Started](`#-getting-started`) | |
| 5. [Folder Structure](`#-folder-structure`) | |
| 6. [Contributing](`#-contributing`) | |
| 7. [License](`#-license`) |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 20-20: Link fragments should be valid
(MD051, link-fragments)
[warning] 21-21: Link fragments should be valid
(MD051, link-fragments)
[warning] 22-22: Link fragments should be valid
(MD051, link-fragments)
[warning] 23-23: Link fragments should be valid
(MD051, link-fragments)
[warning] 24-24: Link fragments should be valid
(MD051, link-fragments)
[warning] 25-25: Link fragments should be valid
(MD051, link-fragments)
[warning] 26-26: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 19 - 26, The Table of Contents links (e.g., link
target "Features") don't match the actual section headers that include emojis
(e.g., "✨ Features", "📦 Tech Stack", "🚀 Project Phases", "⚙️ Getting Started",
"📁 Folder Structure", "🤝 Contributing", "📜 License"), so anchors won't
navigate; fix by either removing the emoji prefixes from those headers so their
slug becomes simple (e.g., "Features") OR update each TOC entry to the exact
anchor slug produced by your renderer (for GitHub-style slugs include the emoji
and hyphen pattern, e.g., the slug for "✨ Features" becomes the
renderer-specific form such as "✨-features" or "-features"); update the TOC
lines (1–7) to match the chosen header form and verify links work in the README
viewer.
Comment on lines
+74
to
+77
| ```bash | ||
| git clone https://github.com/Gooichand/blockchain-evidence.git | ||
| cd blockchain-evidence-1 | ||
| ``` |
Contributor
There was a problem hiding this comment.
Clone instruction mentions different directory name than repository.
The clone command clones blockchain-evidence.git but then cds into blockchain-evidence-1. This will fail unless the user manually renames the directory.
Proposed fix
```bash
git clone https://github.com/Gooichand/blockchain-evidence.git
- cd blockchain-evidence-1
+ cd blockchain-evidence
```📝 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
| ```bash | |
| git clone https://github.com/Gooichand/blockchain-evidence.git | |
| cd blockchain-evidence-1 | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 74 - 77, Update the README clone instructions so the
post-clone directory matches the repository name: change the cd target
referenced in the snippet from "blockchain-evidence-1" to "blockchain-evidence"
(the directory created by git clone
https://github.com/Gooichand/blockchain-evidence.git) to avoid a failing cd
command; update the snippet where the clone and cd commands appear.
Comment on lines
93
to
104
| ``` | ||
| ┌─────────────────┐ | ||
| │ Web Browser │ | ||
| │ (MetaMask + │ | ||
| │ Frontend) │ | ||
| └────────┬────────┘ | ||
| │ | ||
| │ HTTPS | ||
| ▼ | ||
| ┌─────────────────────────────────┐ | ||
| │ Express.js Backend │ | ||
| │ ┌──────────────────────────┐ │ | ||
| │ │ Authentication Layer │ │ | ||
| │ │ (MetaMask/Email) │ │ | ||
| │ └──────────────────────────┘ │ | ||
| │ ┌──────────────────────────┐ │ | ||
| │ │ Role-Based Access │ │ | ||
| │ │ Control (RBAC) │ │ | ||
| │ └──────────────────────────┘ │ | ||
| │ ┌──────────────────────────┐ │ | ||
| │ │ Evidence Processing │ │ | ||
| │ │ (Upload/Watermark) │ │ | ||
| │ └──────────────────────────┘ │ | ||
| │ ┌──────────────────────────┐ │ | ||
| │ │ Real-time Events │ │ | ||
| │ │ (Socket.IO) │ │ | ||
| │ └──────────────────────────┘ │ | ||
| └────┬──────────┬─────────┬───────┘ | ||
| │ │ │ | ||
| │ │ │ | ||
| ▼ ▼ ▼ | ||
| ┌─────────┐ ┌─────────┐ ┌──────────┐ | ||
| │Supabase │ │ IPFS │ │Blockchain│ | ||
| │PostgreSQL│ │(Pinata) │ │(Polygon) │ | ||
| │ +RLS │ │ Storage │ │ Network │ | ||
| └─────────┘ └─────────┘ └──────────┘ | ||
| blockchain-evidence/ | ||
| +-- contracts/ # Solidity contracts | ||
| +-- docs/ # Documentation (deployment, security, etc.) | ||
| +-- public/ # Frontend pages & assets | ||
| +-- controllers/ # Express route handlers | ||
| +-- middleware/ # Express middleware | ||
| +-- services/ # Business logic | ||
| +-- migrations/ # SQL migrations | ||
| +-- tests/ # Jest test suites | ||
| +-- utils/ # Helper modules | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language specifier to fenced code block.
Static analysis flagged the missing language identifier. Adding text or plaintext improves rendering consistency.
Proposed fix
-```
+```text
blockchain-evidence/📝 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
| ``` | |
| ┌─────────────────┐ | |
| │ Web Browser │ | |
| │ (MetaMask + │ | |
| │ Frontend) │ | |
| └────────┬────────┘ | |
| │ | |
| │ HTTPS | |
| ▼ | |
| ┌─────────────────────────────────┐ | |
| │ Express.js Backend │ | |
| │ ┌──────────────────────────┐ │ | |
| │ │ Authentication Layer │ │ | |
| │ │ (MetaMask/Email) │ │ | |
| │ └──────────────────────────┘ │ | |
| │ ┌──────────────────────────┐ │ | |
| │ │ Role-Based Access │ │ | |
| │ │ Control (RBAC) │ │ | |
| │ └──────────────────────────┘ │ | |
| │ ┌──────────────────────────┐ │ | |
| │ │ Evidence Processing │ │ | |
| │ │ (Upload/Watermark) │ │ | |
| │ └──────────────────────────┘ │ | |
| │ ┌──────────────────────────┐ │ | |
| │ │ Real-time Events │ │ | |
| │ │ (Socket.IO) │ │ | |
| │ └──────────────────────────┘ │ | |
| └────┬──────────┬─────────┬───────┘ | |
| │ │ │ | |
| │ │ │ | |
| ▼ ▼ ▼ | |
| ┌─────────┐ ┌─────────┐ ┌──────────┐ | |
| │Supabase │ │ IPFS │ │Blockchain│ | |
| │PostgreSQL│ │(Pinata) │ │(Polygon) │ | |
| │ +RLS │ │ Storage │ │ Network │ | |
| └─────────┘ └─────────┘ └──────────┘ | |
| blockchain-evidence/ | |
| +-- contracts/ # Solidity contracts | |
| +-- docs/ # Documentation (deployment, security, etc.) | |
| +-- public/ # Frontend pages & assets | |
| +-- controllers/ # Express route handlers | |
| +-- middleware/ # Express middleware | |
| +-- services/ # Business logic | |
| +-- migrations/ # SQL migrations | |
| +-- tests/ # Jest test suites | |
| +-- utils/ # Helper modules | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 93 - 104, The README's fenced code block listing the
repo tree (the block starting with "blockchain-evidence/") is missing a language
specifier; update the opening fence to include a language such as `text` or
`plaintext` (e.g., change ``` to ```text) so the code block renders consistently
across static analysis and markdown renderers.
| ## 📜 Code of Conduct | ||
|
|
||
| We are committed to providing a friendly, safe, and welcoming environment. Please review our [CODE_OF_CONDUCT.md](CODE_OF_CONDUCT.md). | ||
| This project is licensed under the MIT License – see the [LICENSE](LICENSE) file for details. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the LICENSE file content matches the MIT claim
echo "=== LICENSE file content ==="
cat LICENSE 2>/dev/null || echo "LICENSE file not found"
echo ""
echo "=== Searching for license references in markdown ==="
rg -i "apache|mit" --type=md -C1
echo ""
echo "=== Checking package.json for license field ==="
cat package.json 2>/dev/null | grep -i "license" || echo "No package.json or license field found"
echo ""
echo "=== Searching for Apache 2.0 in entire codebase ==="
rg -i "apache" --type-list | head -5
rg -i "apache" -l 2>/dev/null | head -10Repository: Gooichand/blockchain-evidence
Length of output: 18546
License change from Apache 2.0 to MIT is incomplete.
The README.md was updated to claim MIT License, but the actual LICENSE file still contains Apache License 2.0 (verified). Additionally, package.json still declares "license": "Apache-2.0". This creates a critical inconsistency between repository metadata and declared license:
- README claims: MIT
- Actual LICENSE file: Apache 2.0
- package.json: Apache-2.0
Either revert the README changes to Apache 2.0, or update the LICENSE file and package.json to MIT to match the README declaration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 119, The README now states MIT but the repository still
contains an Apache-2.0 LICENSE and package.json lists "license": "Apache-2.0";
fix the inconsistency by choosing the intended license and updating all three
places consistently: either revert the README text to reference Apache 2.0, or
replace the LICENSE file contents with the MIT license text and update
package.json's "license" field to "MIT" (and update any license header comments
if present). Ensure the README license line, the LICENSE file contents, and the
package.json "license" property all match exactly.
| --- | ||
|
|
||
| <p align="right"><a href="#-evid-dgc---blockchain-evidence-management-system">Back to Top ↑</a></p> | ||
| *Last updated: March?5?2026* |
Contributor
There was a problem hiding this comment.
Fix character encoding issue in date.
The line contains non-standard characters (likely non-breaking spaces or encoding artifacts): March?5?2026.
Proposed fix
-*Last updated: March?5?2026*
+*Last updated: March 5, 2026*📝 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
| *Last updated: March?5?2026* | |
| *Last updated: March 5, 2026* |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 123, The README contains invalid characters in the date
string "March?5?2026"; replace the non-standard characters with standard ASCII
so it reads "March 5 2026" (or "March 5, 2026" if you prefer a comma), ensuring
you remove any non-breaking spaces or encoding artifacts; locate and update the
exact string "March?5?2026" in the README to use plain spaces and ASCII
punctuation.
…king - Implement total connection limit (1000) and per-IP limit (10) - Add per-connection rate limiting (20 events/minute) - Track active connections and clean up on disconnect - Prevent abuse of WebSocket notifications endpoint
- Update GitHub links to use correct username: deekshi275 - Fixes broken links in contributor onboarding
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server.js (1)
15-24: These limits only apply per Node.js process.
activeConnections,ipConnectionCounts, andeventRateLimitsare all in-memory, so they reset on restart and do not coordinate across multiple app instances. If this service ever runs with horizontal scaling, abuse can be spread across replicas and bypass the intended caps. Consider moving this state to a shared store such as Redis.Also applies to: 34-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.js` around lines 15 - 24, The current in-memory rate limiting and connection tracking (activeConnections, connectionTimestamps, ipConnectionCounts, eventRateLimits and related constants like MAX_CONNECTIONS_TOTAL, MAX_CONNECTIONS_PER_IP, RATE_LIMIT_PER_MINUTE) only work per Node process and will be bypassed under horizontal scaling; refactor these data structures to use a shared store (e.g., Redis) so counts, timestamps and deque-like event windows are persisted and coordinated across instances, replacing Map usage with Redis keys/hashes/streams and updating the connection open/close handlers and rate-check logic to read/update Redis atomically (using INCR/EXPIRE, sorted sets or lists and Lua scripts where needed) to preserve the existing semantics across replicas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server.js`:
- Around line 58-87: The current per-socket limiter in checkRateLimit uses
socket.id so reconnects/fan-outs bypass limits; change the keying of
eventRateLimits from socket.id to a stable client identifier (e.g.,
authenticated wallet ID or socket.handshake.address) and use that stableId in
checkRateLimit (instead of socket.id) and any disconnect cleanup so budgets
persist across reconnects; ensure the code that currently clears budgets on
disconnect (related to the disconnect/join handling) uses the stableId or does
not remove the entry on simple reconnects, and keep references to
RATE_LIMIT_PER_MINUTE and the event name logic intact so behavior and messages
remain the same.
- Around line 117-129: The join handler currently only calls
validateWalletAddress() which allows any client to subscribe to arbitrary wallet
rooms; change the 'join' handler to require and verify proof of ownership (e.g.
a signed message or a server-issued auth token) before calling
connectedUsers.set() and socket.join(): accept a payload like { walletAddress,
signature, token }, verify the signature with a new
verifyWalletSignature(walletAddress, signature) or validate the server token
with verifyJoinToken(token, walletAddress), and only on successful verification
proceed to connectedUsers.set(walletAddress, socket.id) and
socket.join(walletAddress); if verification fails, emit an error and do not
join. Ensure you add/inline the verification helpers and use the same symbols
(socket.on('join'), validateWalletAddress, connectedUsers.set, socket.join) so
reviewers can find the change.
- Around line 135-159: The disconnect handler only removes the first mapping
where connectedUsers maps a wallet to socket.id (the for-loop breaks after one
delete), leaving other wallets still pointing to the dead socket; update the
cleanup to remove all entries that reference the disconnecting socket: iterate
over connectedUsers (or use Array.from(connectedUsers.entries())) and delete
every wallet whose socketId === socket.id (or enforce a single-wallet-per-socket
invariant elsewhere such as in the join handler), ensuring all remaining stale
mappings referencing socket.id are removed; refer to connectedUsers and
socket.id in the disconnect handler to locate the change.
---
Nitpick comments:
In `@server.js`:
- Around line 15-24: The current in-memory rate limiting and connection tracking
(activeConnections, connectionTimestamps, ipConnectionCounts, eventRateLimits
and related constants like MAX_CONNECTIONS_TOTAL, MAX_CONNECTIONS_PER_IP,
RATE_LIMIT_PER_MINUTE) only work per Node process and will be bypassed under
horizontal scaling; refactor these data structures to use a shared store (e.g.,
Redis) so counts, timestamps and deque-like event windows are persisted and
coordinated across instances, replacing Map usage with Redis keys/hashes/streams
and updating the connection open/close handlers and rate-check logic to
read/update Redis atomically (using INCR/EXPIRE, sorted sets or lists and Lua
scripts where needed) to preserve the existing semantics across replicas.
Comment on lines
+58
to
+87
| function checkRateLimit(socket, event) { | ||
| const socketId = socket.id; | ||
| const now = Date.now(); | ||
| const windowMs = 60 * 1000; // 1 minute | ||
|
|
||
| if (!eventRateLimits.has(socketId)) { | ||
| eventRateLimits.set(socketId, new Map()); | ||
| } | ||
|
|
||
| const socketLimits = eventRateLimits.get(socketId); | ||
| if (!socketLimits.has(event)) { | ||
| socketLimits.set(event, []); | ||
| } | ||
|
|
||
| const timestamps = socketLimits.get(event); | ||
|
|
||
| // Remove old timestamps | ||
| while (timestamps.length > 0 && now - timestamps[0] > windowMs) { | ||
| timestamps.shift(); | ||
| } | ||
|
|
||
| if (timestamps.length >= RATE_LIMIT_PER_MINUTE) { | ||
| socket.emit('error', { | ||
| message: `Rate limit exceeded for ${event}. Maximum ${RATE_LIMIT_PER_MINUTE} events per minute.` | ||
| }); | ||
| return false; | ||
| } | ||
|
|
||
| timestamps.push(now); | ||
| return true; |
Contributor
There was a problem hiding this comment.
Per-socket rate limits are easy to bypass.
The limiter is keyed by socket.id, and Line 157 clears that budget on disconnect. A client can reconnect or fan out across sockets to immediately regain a fresh 20-event allowance, so the new join protection does not really constrain abusive callers. Key this to a stable identity such as authenticated wallet or IP, and keep it across reconnects.
Also applies to: 156-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server.js` around lines 58 - 87, The current per-socket limiter in
checkRateLimit uses socket.id so reconnects/fan-outs bypass limits; change the
keying of eventRateLimits from socket.id to a stable client identifier (e.g.,
authenticated wallet ID or socket.handshake.address) and use that stableId in
checkRateLimit (instead of socket.id) and any disconnect cleanup so budgets
persist across reconnects; ensure the code that currently clears budgets on
disconnect (related to the disconnect/join handling) uses the stableId or does
not remove the entry on simple reconnects, and keep references to
RATE_LIMIT_PER_MINUTE and the event name logic intact so behavior and messages
remain the same.
Comment on lines
117
to
129
| socket.on('join', (walletAddress) => { | ||
| // Rate limit the join event | ||
| if (!checkRateLimit(socket, 'join')) { | ||
| return; | ||
| } | ||
|
|
||
| if (validateWalletAddress(walletAddress)) { | ||
| connectedUsers.set(walletAddress, socket.id); | ||
| socket.join(walletAddress); | ||
| console.log(`User ${walletAddress} joined notifications`); | ||
| } else { | ||
| socket.emit('error', { message: 'Invalid wallet address' }); | ||
| } |
Contributor
There was a problem hiding this comment.
Authenticate wallet-room joins, not just wallet syntax.
validateWalletAddress() only proves the string is well-formed. Any client can call join with someone else’s wallet address and subscribe to that room, which turns the notifications channel into an authorization bypass. Gate connectedUsers.set() / socket.join() behind proof of wallet ownership or a server-issued authenticated token.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server.js` around lines 117 - 129, The join handler currently only calls
validateWalletAddress() which allows any client to subscribe to arbitrary wallet
rooms; change the 'join' handler to require and verify proof of ownership (e.g.
a signed message or a server-issued auth token) before calling
connectedUsers.set() and socket.join(): accept a payload like { walletAddress,
signature, token }, verify the signature with a new
verifyWalletSignature(walletAddress, signature) or validate the server token
with verifyJoinToken(token, walletAddress), and only on successful verification
proceed to connectedUsers.set(walletAddress, socket.id) and
socket.join(walletAddress); if verification fails, emit an error and do not
join. Ensure you add/inline the verification helpers and use the same symbols
(socket.on('join'), validateWalletAddress, connectedUsers.set, socket.join) so
reviewers can find the change.
Comment on lines
135
to
+159
| socket.on('disconnect', () => { | ||
| // Cleanup connection tracking | ||
| if (activeConnections.has(socket.id)) { | ||
| const { ip } = activeConnections.get(socket.id); | ||
| activeConnections.delete(socket.id); | ||
| const currentCount = ipConnectionCounts.get(ip) || 0; | ||
| if (currentCount > 1) { | ||
| ipConnectionCounts.set(ip, currentCount - 1); | ||
| } else { | ||
| ipConnectionCounts.delete(ip); | ||
| } | ||
| } | ||
|
|
||
| // Cleanup from connectedUsers | ||
| for (const [wallet, socketId] of connectedUsers.entries()) { | ||
| if (socketId === socket.id) { | ||
| connectedUsers.delete(wallet); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Cleanup rate limits | ||
| if (eventRateLimits.has(socket.id)) { | ||
| eventRateLimits.delete(socket.id); | ||
| } |
Contributor
There was a problem hiding this comment.
Disconnect cleanup leaves stale connectedUsers entries.
Line 152 breaks after deleting the first wallet mapped to this socket. If the same socket calls join more than once, the remaining wallet entries survive disconnect and keep pointing at a dead socket id. Either enforce one wallet per socket or delete all matches here.
Proposed fix
- for (const [wallet, socketId] of connectedUsers.entries()) {
- if (socketId === socket.id) {
- connectedUsers.delete(wallet);
- break;
- }
- }
+ for (const [wallet, socketId] of connectedUsers.entries()) {
+ if (socketId === socket.id) {
+ connectedUsers.delete(wallet);
+ }
+ }📝 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
| socket.on('disconnect', () => { | |
| // Cleanup connection tracking | |
| if (activeConnections.has(socket.id)) { | |
| const { ip } = activeConnections.get(socket.id); | |
| activeConnections.delete(socket.id); | |
| const currentCount = ipConnectionCounts.get(ip) || 0; | |
| if (currentCount > 1) { | |
| ipConnectionCounts.set(ip, currentCount - 1); | |
| } else { | |
| ipConnectionCounts.delete(ip); | |
| } | |
| } | |
| // Cleanup from connectedUsers | |
| for (const [wallet, socketId] of connectedUsers.entries()) { | |
| if (socketId === socket.id) { | |
| connectedUsers.delete(wallet); | |
| break; | |
| } | |
| } | |
| // Cleanup rate limits | |
| if (eventRateLimits.has(socket.id)) { | |
| eventRateLimits.delete(socket.id); | |
| } | |
| socket.on('disconnect', () => { | |
| // Cleanup connection tracking | |
| if (activeConnections.has(socket.id)) { | |
| const { ip } = activeConnections.get(socket.id); | |
| activeConnections.delete(socket.id); | |
| const currentCount = ipConnectionCounts.get(ip) || 0; | |
| if (currentCount > 1) { | |
| ipConnectionCounts.set(ip, currentCount - 1); | |
| } else { | |
| ipConnectionCounts.delete(ip); | |
| } | |
| } | |
| // Cleanup from connectedUsers | |
| for (const [wallet, socketId] of connectedUsers.entries()) { | |
| if (socketId === socket.id) { | |
| connectedUsers.delete(wallet); | |
| } | |
| } | |
| // Cleanup rate limits | |
| if (eventRateLimits.has(socket.id)) { | |
| eventRateLimits.delete(socket.id); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server.js` around lines 135 - 159, The disconnect handler only removes the
first mapping where connectedUsers maps a wallet to socket.id (the for-loop
breaks after one delete), leaving other wallets still pointing to the dead
socket; update the cleanup to remove all entries that reference the
disconnecting socket: iterate over connectedUsers (or use
Array.from(connectedUsers.entries())) and delete every wallet whose socketId ===
socket.id (or enforce a single-wallet-per-socket invariant elsewhere such as in
the join handler), ensuring all remaining stale mappings referencing socket.id
are removed; refer to connectedUsers and socket.id in the disconnect handler to
locate the change.
Owner
|
what the issue no your working |
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
2 participants
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.
This PR fixes inconsistent logo usage across the frontend and replaces the outdated, cluttered README with a clean, structured document:
Summary by CodeRabbit
Documentation
Chores
New Features