Skip to content

feat: add in-chat media gallery with lightbox image viewer Closes #499 #536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
darshil2032007 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add in-chat media gallery with lightbox image viewer Closes #499 #536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion backend/src/lib/socket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,23 @@ import Message from "../models/message.model.js";
import User from "../models/user.model.js";

const app = express();
// Cache to track last DB update time per user
const lastDbUpdateCache = new Map();

// Throttled lastSeen updater - max once every 30 seconds per user
const throttledUpdateLastSeen = async (userId) => {
const now = Date.now();
const lastUpdate = lastDbUpdateCache.get(userId) || 0;

if (now - lastUpdate < 30000) return; // 30 second throttle

lastDbUpdateCache.set(userId, now);
try {
await User.findByIdAndUpdate(userId, { lastSeen: new Date() });
} catch (err) {
console.error("Error updating lastSeen:", err);
}
Comment on lines +19 to +24

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rollback throttle cache when lastSeen DB write fails.

The cache timestamp is set before the DB write, but on error it is never reverted. That suppresses retries for 30s despite no successful update, which can leave lastSeen stale.

Suggested fix 
 const throttledUpdateLastSeen = async (userId) => {
     const now = Date.now();
     const lastUpdate = lastDbUpdateCache.get(userId) || 0;
     
     if (now - lastUpdate < 30000) return; // 30 second throttle
     
     lastDbUpdateCache.set(userId, now);
     try {
         await User.findByIdAndUpdate(userId, { lastSeen: new Date() });
     } catch (err) {
+        lastDbUpdateCache.delete(userId); // allow immediate retry after transient failure
         console.error("Error updating lastSeen:", err);
     }
 };
📝 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
lastDbUpdateCache.set(userId, now);
try {
await User.findByIdAndUpdate(userId, { lastSeen: new Date() });
} catch (err) {
console.error("Error updating lastSeen:", err);
}
lastDbUpdateCache.set(userId, now);
try {
await User.findByIdAndUpdate(userId, { lastSeen: new Date() });
} catch (err) {
lastDbUpdateCache.delete(userId); // allow immediate retry after transient failure
console.error("Error updating lastSeen:", err);
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/src/lib/socket.js` around lines 19 - 24, The cache is being updated
before the DB write so on failure the timestamp remains and blocks retries; in
the block around lastDbUpdateCache.set(userId, now) and User.findByIdAndUpdate,
capture the previous cache value (e.g., prev = lastDbUpdateCache.get(userId))
before calling set, then if User.findByIdAndUpdate throws restore the previous
state (either lastDbUpdateCache.set(userId, prev) if prev existed or
lastDbUpdateCache.delete(userId) if not) inside the catch, and keep the existing
console.error logging; reference lastDbUpdateCache, userId, now, and
User.findByIdAndUpdate when making the change.

};
const server = http.createServer(app);

const io = new Server(server, {
Expand Down Expand Up @@ -70,7 +87,6 @@ const canCommunicate = async (senderId, receiverId) => {
}
};

io.on("connection", (socket) => {
const getActiveContacts = async (userId) => {
try {
const [senders, receivers] = await Promise.all([
Expand Down
20 changes: 20 additions & 0 deletions backend/src/routes/message.route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,26 @@ router.get("/:id", protectRoute, getMessages);
router.post("/send/:id", protectRoute, sendMessage);
router.post("/:id/react", protectRoute, reactToMessage);
router.delete("/:id", protectRoute, deleteMessage);
// Media Gallery Route
router.get("/:id/media", protectRoute, async (req, res) => {
try {
const myId = req.user._id;
const theirId = req.params.id;

const mediaMessages = await Message.find({
Comment on lines +19 to +24

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Media endpoint will 500 due auth field mismatch and missing model import.

protectRoute provides req.userId, but this handler reads req.user._id, and Message is used without an import. Either condition can break the route at runtime.

Suggested fix 
 import express from "express";
 import protectRoute from "../middleware/auth.middleware.js";
+import Message from "../models/message.model.js";
@@
-    const myId = req.user._id;
+    const myId = req.userId;
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/src/routes/message.route.js` around lines 19 - 24, The GET /:id/media
route handler is using req.user._id and the Message model without importing it;
change the handler to read the authenticated user id from req.userId (provided
by protectRoute) instead of req.user._id, and add the missing import for the
Message model (e.g., import or require Message at the top of the file). Update
references inside the router.get("/:id/media", protectRoute, async (req, res) =>
{ ... }) handler to use myId = req.userId and ensure Message.find(...) uses the
imported Message model.

$or: [
{ senderId: myId, receiverId: theirId },
{ senderId: theirId, receiverId: myId },
],
image: { $ne: null, $exists: true },

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Current media filter still includes non-image messages.

image: { $ne: null, $exists: true } matches empty strings, and your schema defaults image to "". That means text-only messages can leak into this endpoint.

Suggested fix 
-      image: { $ne: null, $exists: true },
+      image: { $nin: [null, ""] },
📝 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
image: { $ne: null, $exists: true },
image: { $nin: [null, ""] },
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/src/routes/message.route.js` at line 29, The current Mongo filter
image: { $ne: null, $exists: true } allows empty strings; update the query that
builds the media filter (the object using the image field in
backend/src/routes/message.route.js) to explicitly exclude empty strings and
non-string types — e.g. change to image: { $exists: true, $ne: "", $type:
"string" } (or at minimum add $ne: ""), so only messages with a real image value
are returned.

})
.select("image createdAt senderId")
.sort({ createdAt: -1 });

res.status(200).json(mediaMessages);
} catch (error) {
res.status(500).json({ error: "Internal server error" });
}
});
export default router;

Loading