Skip to content

Fix chat room names by enriching with user-service data#14

Open
keshav-sudo wants to merge 10 commits into
devfrom
keshav
Open

Fix chat room names by enriching with user-service data#14
keshav-sudo wants to merge 10 commits into
devfrom
keshav

Conversation

@keshav-sudo
Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings January 16, 2026 14:50
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes chat room names by enriching them with user data from the user-service when names are missing or set to "Unknown". The implementation adds a new user-client service to fetch user details and updates the room controller to enrich room data with proper user names before returning to the client.

Changes:

  • Added a new user-client service to fetch user details from the user-service via HTTP
  • Modified the room controller to detect and enrich missing user names using the user-client service
  • Added production environment configuration file with credentials
  • Updated GitHub callback URL in .env.example
  • Removed development .env file

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
chat-service/src/services/user-client.service.ts New service for fetching user data from user-service with fallback for local development
chat-service/src/api/controllers/room.controller.ts Enhanced getRooms endpoint to fetch and enrich missing user names from user-service
.env.production Added production environment configuration (contains sensitive credentials)
.env.example Updated GitHub callback URL port from 8000 to 80
.env Removed development environment file
Comments suppressed due to low confidence (1)

.env:1

  • While this file is being deleted in the PR, it contains production credentials that were previously committed to the repository. These credentials (MongoDB connection string, GitHub OAuth secrets, JWT secrets) are now exposed in the git history and should be rotated immediately. Simply removing the file does not remove it from git history.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .env.production
Comment on lines +2 to +36
# PRODUCTION Environment Configuration
# ============================================
# IMPORTANT: Keep this file secure and never commit to git!

# ==================== DATABASE ====================
DATABASE_URL=mongodb+srv://thesharmakeshav:TFJUWDRi46dbR5TB@cluster0.kegg0.mongodb.net/verifydev?retryWrites=true&w=majority
CHAT_DATABASE_URL=mongodb+srv://thesharmakeshav:TFJUWDRi46dbR5TB@cluster0.kegg0.mongodb.net/verifydev?retryWrites=true&w=majority

# ==================== GITHUB OAUTH ====================
GITHUB_CLIENT_ID=Ov23li8KrcXPVTwLxWUE
GITHUB_CLIENT_SECRET=34d8078e6d9a86b8daadf94aa9c36c053a276ba7
GITHUB_CALLBACK_URL=https://api.verifydev.me/api/v1/auth/github/callback
GITHUB_TOKEN=

# ==================== JWT SECRETS ====================
# PRODUCTION: Generate strong secrets with: openssl rand -base64 32
JWT_ACCESS_SECRET=prod_access_secret_CHANGE_THIS_32chars_min!
JWT_REFRESH_SECRET=prod_refresh_secret_CHANGE_THIS_32chars!

# ==================== REDIS ====================
REDIS_HOST=redis
REDIS_PORT=6379
REDIS_PASSWORD=

# ==================== RABBITMQ ====================
RABBITMQ_USER=verifydev
RABBITMQ_PASS=verifydev_prod_password_CHANGE_THIS
RABBITMQ_URL=amqp://verifydev:verifydev_prod_password_CHANGE_THIS@rabbitmq:5672

# ==================== CORS ====================
CORS_ORIGIN=https://verifydev.me,https://www.verifydev.me,https://api.verifydev.me

# ==================== ENVIRONMENT ====================
NODE_ENV=production
FRONTEND_URL=https://verifydev.me
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Production credentials (database connection strings, GitHub OAuth secrets) are being committed to the repository. This is a critical security vulnerability. The .gitignore file explicitly excludes .env files but allows .env.production. These credentials should be stored in a secure secrets management system (e.g., GitHub Secrets, AWS Secrets Manager, HashiCorp Vault) and never committed to version control. Remove this file and add .env.production to .gitignore.

Suggested change
# PRODUCTION Environment Configuration
# ============================================
# IMPORTANT: Keep this file secure and never commit to git!
# ==================== DATABASE ====================
DATABASE_URL=mongodb+srv://thesharmakeshav:TFJUWDRi46dbR5TB@cluster0.kegg0.mongodb.net/verifydev?retryWrites=true&w=majority
CHAT_DATABASE_URL=mongodb+srv://thesharmakeshav:TFJUWDRi46dbR5TB@cluster0.kegg0.mongodb.net/verifydev?retryWrites=true&w=majority
# ==================== GITHUB OAUTH ====================
GITHUB_CLIENT_ID=Ov23li8KrcXPVTwLxWUE
GITHUB_CLIENT_SECRET=34d8078e6d9a86b8daadf94aa9c36c053a276ba7
GITHUB_CALLBACK_URL=https://api.verifydev.me/api/v1/auth/github/callback
GITHUB_TOKEN=
# ==================== JWT SECRETS ====================
# PRODUCTION: Generate strong secrets with: openssl rand -base64 32
JWT_ACCESS_SECRET=prod_access_secret_CHANGE_THIS_32chars_min!
JWT_REFRESH_SECRET=prod_refresh_secret_CHANGE_THIS_32chars!
# ==================== REDIS ====================
REDIS_HOST=redis
REDIS_PORT=6379
REDIS_PASSWORD=
# ==================== RABBITMQ ====================
RABBITMQ_USER=verifydev
RABBITMQ_PASS=verifydev_prod_password_CHANGE_THIS
RABBITMQ_URL=amqp://verifydev:verifydev_prod_password_CHANGE_THIS@rabbitmq:5672
# ==================== CORS ====================
CORS_ORIGIN=https://verifydev.me,https://www.verifydev.me,https://api.verifydev.me
# ==================== ENVIRONMENT ====================
NODE_ENV=production
FRONTEND_URL=https://verifydev.me
# PRODUCTION Environment Configuration (TEMPLATE)
# ============================================
# IMPORTANT:
# - Do NOT store real production secrets in this file in the repository.
# - Provide the actual values via your deployment environment / secrets manager.
# ==================== DATABASE ====================
# MongoDB connection strings (set real values in production environment)
DATABASE_URL=__SET_IN_PRODUCTION__
CHAT_DATABASE_URL=__SET_IN_PRODUCTION__
# ==================== GITHUB OAUTH ====================
# GitHub OAuth configuration (set real values in production environment)
GITHUB_CLIENT_ID=__SET_IN_PRODUCTION__
GITHUB_CLIENT_SECRET=__SET_IN_PRODUCTION__
GITHUB_CALLBACK_URL=__SET_IN_PRODUCTION__
GITHUB_TOKEN=__SET_IN_PRODUCTION__
# ==================== JWT SECRETS ====================
# PRODUCTION: Generate strong secrets with: openssl rand -base64 32
JWT_ACCESS_SECRET=__SET_IN_PRODUCTION__
JWT_REFRESH_SECRET=__SET_IN_PRODUCTION__
# ==================== REDIS ====================
REDIS_HOST=__SET_IN_PRODUCTION__
REDIS_PORT=6379
REDIS_PASSWORD=__SET_IN_PRODUCTION__
# ==================== RABBITMQ ====================
RABBITMQ_USER=__SET_IN_PRODUCTION__
RABBITMQ_PASS=__SET_IN_PRODUCTION__
RABBITMQ_URL=__SET_IN_PRODUCTION__
# ==================== CORS ====================
# Comma-separated list of allowed origins
CORS_ORIGIN=__SET_IN_PRODUCTION__
# ==================== ENVIRONMENT ====================
NODE_ENV=production
FRONTEND_URL=__SET_IN_PRODUCTION__

Copilot uses AI. Check for mistakes.
// Default to localhost/docker service name if not configured
// We try both service name (docker) and localhost (local dev)
// Ideally this comes from config
const userServiceUrl = process.env.USER_SERVICE_URL || 'http://user-service:3002';
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The USER_SERVICE_URL environment variable is not defined in the config file (chat-service/src/config/index.ts) but is accessed directly via process.env. For consistency with the rest of the codebase, this should be added to the centralized config object to maintain a single source of truth for configuration values.

Suggested change
const userServiceUrl = process.env.USER_SERVICE_URL || 'http://user-service:3002';
const userServiceUrl = (config as any).userServiceUrl || 'http://user-service:3002';

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +42

const response = await fetch(`${userServiceUrl}/api/v1/users/${userId}/public`, {
method: 'GET',
headers: {
'Content-Type': 'application/json',
// Add internal secret if needed for bypass?
// For public profile, it should be open or require valid token
}
});

if (!response.ok) {
// Fallback for local development if running outside docker
if (userServiceUrl.includes('user-service')) {
const localUrl = 'http://localhost:3002';
const localRes = await fetch(`${localUrl}/api/v1/users/${userId}/public`);
if (localRes.ok) {
const data = await localRes.json() as { success: boolean; data: User };
return data.data;
}
}
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The fallback mechanism makes a second sequential HTTP request when the first fails, doubling the latency in failure scenarios. This nested try-catch approach also lacks error logging for the fallback attempt. Consider using Promise.race() with both URLs simultaneously or at minimum add error logging for debugging when fallbacks occur.

Suggested change
const response = await fetch(`${userServiceUrl}/api/v1/users/${userId}/public`, {
method: 'GET',
headers: {
'Content-Type': 'application/json',
// Add internal secret if needed for bypass?
// For public profile, it should be open or require valid token
}
});
if (!response.ok) {
// Fallback for local development if running outside docker
if (userServiceUrl.includes('user-service')) {
const localUrl = 'http://localhost:3002';
const localRes = await fetch(`${localUrl}/api/v1/users/${userId}/public`);
if (localRes.ok) {
const data = await localRes.json() as { success: boolean; data: User };
return data.data;
}
}
const headers = {
'Content-Type': 'application/json',
// Add internal secret if needed for bypass?
// For public profile, it should be open or require valid token
} as const;
const primaryUrl = `${userServiceUrl}/api/v1/users/${userId}/public`;
// If we're targeting the docker service name, also try localhost concurrently
if (userServiceUrl.includes('user-service')) {
const localUrl = 'http://localhost:3002';
const fallbackUrl = `${localUrl}/api/v1/users/${userId}/public`;
const fetchWithLabel = async (
url: string,
source: 'primary' | 'fallback'
): Promise<{ res: Response | null; source: 'primary' | 'fallback'; error: unknown | null }> => {
try {
const res = await fetch(url, {
method: 'GET',
headers
});
return { res, source, error: null };
} catch (error) {
logger.error({ userId, error, source }, 'User service request failed');
return { res: null, source, error };
}
};
const { res } = await Promise.race([
fetchWithLabel(primaryUrl, 'primary'),
fetchWithLabel(fallbackUrl, 'fallback')
]);
if (!res || !res.ok) {
return null;
}
const data = await res.json() as { success: boolean; data: User };
return data.data; // Assuming standard ApiResponse structure { success: true, data: User }
}
// When not using the docker service name, just call the configured URL directly
const response = await fetch(primaryUrl, {
method: 'GET',
headers
});
if (!response.ok) {

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +29
// Add internal secret if needed for bypass?
// For public profile, it should be open or require valid token
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

These comments suggest uncertainty about the authentication/authorization requirements for the endpoint. This ambiguity could lead to security issues if the endpoint requires authentication but none is provided. The comment should be clarified or removed, and the authentication requirements should be documented or implemented.

Suggested change
// Add internal secret if needed for bypass?
// For public profile, it should be open or require valid token
// Public profile endpoint is intentionally unauthenticated; no auth headers are sent.
// If this endpoint's auth requirements change, update this client to match that contract.

Copilot uses AI. Check for mistakes.
const users = new Map<string, User>();

// Limit concurrency if needed, but for now specific parallel fetch
const promises = uniqueIds.map(id => this.getUser(id));
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The batch fetch creates parallel requests for each user ID without any concurrency limiting. For large user lists, this could overwhelm the user-service or exhaust connection pools. Consider implementing a concurrency limit using a library like p-limit or batching the requests (e.g., 10 at a time).

Copilot uses AI. Check for mistakes.
let userMap = new Map();
if (missingNameIds.size > 0) {
try {
const { userClient } = await import('../../services/user-client.service.js');
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Using dynamic imports in a request handler is unconventional and adds unnecessary overhead on every request where names are missing. The module should be imported statically at the top of the file like other service dependencies (chatRoomService, messageService, presenceService) for better performance and consistency with the codebase patterns.

Copilot uses AI. Check for mistakes.

// Collect IDs of users with missing names
const missingNameIds = new Set<string>();
rooms.forEach((room: any) => {
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The type annotation 'any' bypasses TypeScript's type safety. The room parameter should be typed as ChatRoom (already imported via chatRoomService) to maintain type safety and catch potential errors at compile time.

Copilot uses AI. Check for mistakes.
// Enrich with unread counts and otherParticipant details
const enrichedRooms = rooms.map((room: any) => {
const otherParticipant = chatRoomService.getOtherParticipant(room, userId);
const otherParticipant = chatRoomService.getOtherParticipant(room, userId) as any;
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Casting to 'any' removes type safety and allows arbitrary property assignment. Define a proper interface for the otherParticipant object that includes the optional avatarUrl property, or extend the return type of getOtherParticipant to be more specific.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,73 @@
import { config } from '../config/index.js';
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Unused import config.

Suggested change
import { config } from '../config/index.js';

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants