Feat/sse tickets#252
Conversation
…ces, including models, controllers, routes, and tests.
…et status to 'in progress' upon new messages.
… management to use `req.user.username` for sender and updater, and reorder admin status check.
…ents (SSE) using a new stream manager utility.
Summary of ChangesHello @VitraagVigyaan, 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 introduces a robust, real-time ticket management system to facilitate communication between users and administrators. It provides dedicated API endpoints for creating, viewing, and interacting with support tickets from both client and admin perspectives. The core real-time messaging functionality is powered by Server-Sent Events (SSE), allowing for instant updates as messages are exchanged. New database models ( 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. 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
|
![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 a new ticketing system with Server-Sent Events (SSE) for real-time message streaming. The implementation includes new models, controllers, routes, and tests. While the feature is well-structured, there are several critical issues that need to be addressed before merging. These include a security vulnerability with permissive CORS settings, use of insufficient randomness for generating unique ticket IDs which could lead to collisions, and several bugs that will cause runtime errors or test failures, such as a missing import and inconsistent API responses. There are also some medium-severity issues related to code cleanliness and maintainability. Additionally, socket.io is introduced but does not appear to be used by the new ticketing feature, which might indicate either incomplete work or dead code.
| await Ticket.create({ | ||
| id: generateTicketId(), | ||
| issued_by: cardno, | ||
| service, | ||
| description, | ||
| os, | ||
| app_version | ||
| }); | ||
|
|
||
| res.status(201).send({ | ||
| message: 'Successfully created ticket' | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The API response for this endpoint is inconsistent with the tests and likely the rest of the application. The tests expect a { status: 'success', data: ... } format, but this endpoint returns { message: '...' }. This will cause tests to fail. The created ticket should also be returned in the response. This issue applies to other new endpoints in this file as well.
| await Ticket.create({ | |
| id: generateTicketId(), | |
| issued_by: cardno, | |
| service, | |
| description, | |
| os, | |
| app_version | |
| }); | |
| res.status(201).send({ | |
| message: 'Successfully created ticket' | |
| }); | |
| }; | |
| const newTicket = await Ticket.create({ | |
| id: generateTicketId(), | |
| issued_by: cardno, | |
| service, | |
| description, | |
| os, | |
| app_version | |
| }); | |
| res.status(201).send({ | |
| status: 'success', | |
| data: newTicket | |
| }); | |
| }; |
| res.status(201).send({ | ||
| message: MSG_UPDATE_SUCCESSFUL | ||
| }); |
There was a problem hiding this comment.
The API response for this endpoint is inconsistent with the tests. It returns only a message, but the tests expect a response format of { status: 'success', data: ... }, including the newly created message. This will cause tests to fail. The response should be updated to include the status and the new message data.
res.status(201).send({
status: 'success',
message: MSG_UPDATE_SUCCESSFUL,
data: newMessage
});| const generateTicketId = () => { | ||
| return crypto.randomBytes(4).toString('hex').toUpperCase(); | ||
| }; |
There was a problem hiding this comment.
The generateTicketId function uses crypto.randomBytes(4), which only provides 32 bits of entropy. This is insufficient for a primary key and creates a significant risk of collisions as the number of tickets grows, potentially leading to data corruption. It is highly recommended to use a more robust method for generating unique IDs, such as crypto.randomUUID().
const generateTicketId = () => {
return crypto.randomUUID();
};| import { | ||
| createTicket, | ||
| getTicketDetails, | ||
| addMessage, | ||
| resolveTicket, | ||
| streamTicketMessages | ||
| } from '../../controllers/client/ticket.controller.js'; |
There was a problem hiding this comment.
The getTickets function is used in a route on line 17, but it is not imported from the controller. This will cause a ReferenceError at runtime.
| import { | |
| createTicket, | |
| getTicketDetails, | |
| addMessage, | |
| resolveTicket, | |
| streamTicketMessages | |
| } from '../../controllers/client/ticket.controller.js'; | |
| import { | |
| createTicket, | |
| getTickets, | |
| getTicketDetails, | |
| addMessage, | |
| resolveTicket, | |
| streamTicketMessages | |
| } from '../../controllers/client/ticket.controller.js'; |
| cors: { | ||
| origin: '*', // restrict later if needed | ||
| methods: ['GET', 'POST'] | ||
| } |
There was a problem hiding this comment.
The CORS configuration for socket.io is overly permissive with origin: '*'. This allows any website to connect to your socket.io server, which is a significant security risk. It is strongly recommended to restrict the origin to a specific list of allowed domains.
| cors: { | |
| origin: '*', // restrict later if needed | |
| methods: ['GET', 'POST'] | |
| } | |
| cors: { | |
| origin: ['https://your-client-app.com', 'https://your-admin-app.com'], // TODO: Replace with actual client domains | |
| methods: ['GET', 'POST'] | |
| } |
| io.on('connection', socket => { | ||
| console.log('Admin connected:', socket.id); | ||
|
|
||
| socket.on('join_admin', () => { | ||
| socket.join('admins'); | ||
| }); | ||
|
|
||
| socket.on('disconnect', () => { | ||
| console.log('Disconnected:', socket.id); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
console.log is used for logging. It's better to use the application-wide logger for consistency and to allow for configurable log levels and destinations in different environments. Please import and use the application's logger here.
| io.on('connection', socket => { | |
| console.log('Admin connected:', socket.id); | |
| socket.on('join_admin', () => { | |
| socket.join('admins'); | |
| }); | |
| socket.on('disconnect', () => { | |
| console.log('Disconnected:', socket.id); | |
| }); | |
| }); | |
| io.on('connection', socket => { | |
| logger.info('Admin connected:', socket.id); | |
| socket.on('join_admin', () => { | |
| socket.join('admins'); | |
| }); | |
| socket.on('disconnect', () => { | |
| logger.info('Disconnected:', socket.id); | |
| }); | |
| }); |
| // export const getAllTickets = async (req, res) => { | ||
| // const { status, service } = req.query; | ||
| // const where = {}; | ||
|
|
||
| // if (status) where.status = status; | ||
| // if (service) where.service = service; | ||
|
|
||
| // const tickets = await Ticket.findAll({ | ||
| // where, | ||
| // order: [['createdAt', 'DESC']] | ||
| // }); | ||
|
|
||
| // res.status(200).json({ | ||
| // status: 'success', | ||
| // message: MSG_FETCH_SUCCESSFUL, | ||
| // data: tickets | ||
| // }); | ||
| // }; |
There was a problem hiding this comment.
This large block of commented-out code should be removed. Version control history can be used to track previous implementations if needed, and removing dead code keeps the codebase clean and easier to maintain.
| }, | ||
| status: { | ||
| type: DataTypes.ENUM, | ||
| values: [STATUS_OPEN, STATUS_INPROGRESS, 'resolved', STATUS_CLOSED], |
There was a problem hiding this comment.
The status 'resolved' is hardcoded in the ENUM values. For better maintainability and consistency, it should be defined as a constant (e.g., STATUS_RESOLVED) in config/constants.js and imported here, similar to the other status values.
| values: [STATUS_OPEN, STATUS_INPROGRESS, 'resolved', STATUS_CLOSED], | |
| values: [STATUS_OPEN, STATUS_INPROGRESS, STATUS_RESOLVED, STATUS_CLOSED], |
…r handling by removing disconnected clients on write failure.
3 participants
No description provided.