merge modern features into home page redesign (needs work)#36
Conversation
massive changes to message system, and threads now live update
|
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 You can disable this status message by setting the WalkthroughThis update introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SocketServer
participant AuthModule
participant Database
Client->>SocketServer: Emit "clatter.channel.message.send" with { message, method: "modern", token }
SocketServer->>AuthModule: Validate bearer token
AuthModule-->>SocketServer: Return user session
SocketServer->>Database: Create message record (content, sender, room, parent)
Database-->>SocketServer: Return new message ID
SocketServer->>Client: Emit confirmation with new message ID
SocketServer->>Other Clients: Broadcast message to room
sequenceDiagram
participant App
participant AuthModule
participant EmailService
App->>AuthModule: Request email verification
AuthModule->>EmailService: sendVerificationEmail(user, verificationUrl)
EmailService-->>AuthModule: Email sent confirmation
AuthModule-->>App: Notify completion
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
app/auth.ts (1)
23-232: Consider extracting HTML email template.The email template is very long (200+ lines) and embedded directly in the code. Consider extracting it to a separate template file for better maintainability and readability.
You could create a separate email template file and load it, or use a template engine like Handlebars for better email template management.
app/client/pages/channelpage.html (1)
71-78: Potential DOM performance issue with repeated queries.The code performs DOM queries inside the message receive handler for every message with a parent. This could impact performance when handling multiple messages rapidly.
Consider caching DOM elements or debouncing these operations:
- } else { - var refmessage = $("#messagearea").find("[data-messageid='" + element.parentmessageid + "']") - if(refmessage.attr("isthread") != "true") { - refmessage.attr("isthread", "true") - refmessage.find(".mstack").append(` - <div class="threadviewbutton" onclick="currentmessageid = '` + element.parentmessageid + `; NavigatePage('channelthreadpage')">View Thread</div> - `) - } + } else { + var refmessage = $("#messagearea").find("[data-messageid='" + element.parentmessageid + "']") + if(refmessage.length && refmessage.attr("isthread") !== "true") { + refmessage.attr("isthread", "true") + refmessage.find(".mstack").append(` + <div class="threadviewbutton" onclick="currentmessageid = '` + element.parentmessageid + `'; NavigatePage('channelthreadpage')">View Thread</div> + `) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/auth.js(2 hunks)app/auth.ts(4 hunks)app/client/global/styles.css(1 hunks)app/client/pages/channelpage.html(7 hunks)app/client/pages/channelthreadpage.html(2 hunks)app/server.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/auth.ts (1)
app/auth.js (1)
resend(11-11)
🪛 Biome (1.9.4)
app/server.js
[error] 266-266: Shouldn't redeclare 'parentid'. Consider to delete it or rename it.
'parentid' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (10)
app/client/global/styles.css (1)
32-32: Excellent viewport height improvement!The change from
100vhto100dvh(dynamic viewport height) is a great enhancement for mobile compatibility. This ensures the body height adapts properly when mobile browser UI elements (like address bars) appear or disappear, providing a more consistent user experience.app/auth.js (2)
2-2: Bearer plugin import looks good.The addition of the
bearerplugin to the import statement is consistent with the corresponding TypeScript auth configuration and supports the new token-based authentication flow.
469-470: Bearer plugin configuration is properly implemented.The bearer plugin is correctly added to the plugins array, enabling token-based authentication that aligns with the socket-based message sending mechanism introduced in the client pages.
app/auth.ts (3)
2-2: Bearer plugin import is consistent.The addition of the
bearerplugin import aligns with the corresponding changes inapp/auth.jsand supports the enhanced authentication flow.
457-457: Good email sender address update.The change to use
noreply@clatterauth.quntem.co.ukfollows email best practices for automated emails and is consistent with the verification email configuration.
469-470: Bearer plugin correctly configured.The bearer plugin is properly added to support token-based authentication, which aligns with the socket-based messaging implementation in the client pages.
app/client/pages/channelthreadpage.html (2)
19-19: Good variable declaration practice.Using
var socket = io()in the catch block instead of reassigning is correct and prevents potential scope issues.
232-235:Details
✅ Verification successful
Verify server-side token validation.
The new socket message payload includes authentication token and modern method fields. Ensure that the server properly validates the bearer token before processing these messages to prevent unauthorized message sending.
🏁 Script executed:
#!/bin/bash # Description: Verify that the server validates bearer tokens for socket message sends # Search for socket message handling with token validation rg -A 10 -B 5 "clatter\.channel\.message\.send"Length of output: 3857
🏁 Script executed:
#!/bin/bash # Fetch extended context around message send handler rg -n "socket.on(\"clatter.channel.message.send\"" -A 20 -B 20 app/server.jsLength of output: 177
🏁 Script executed:
#!/bin/bash # Retrieve full context of the socket message send handler with proper escaping rg -n 'socket.on\("clatter\.channel\.message\.send"' -A 30 -B 5 app/server.jsLength of output: 1351
Server-side token validation is already implemented
The handler in app/server.js (lines 257–263) calls auth.api.getSession with the Bearer token before creating any message, ensuring that only valid sessions can send messages:• app/server.js:
if (argjson.method === "modern") { const session = await auth.api.getSession({ headers: new Headers({ authorization: "Bearer " + argjson.token }) }); // …message creation uses session.session.userId… }No further changes are needed here.
app/client/pages/channelpage.html (2)
183-185: Verify token security and add error handling for socket events.The authentication token is being sent via socket events. Ensure this approach maintains security and add error handling for failed message sends.
The client should handle potential authentication failures from the server. Consider adding error event listeners:
socket.on("clatter.channel.message.send.error", (error) => { console.error("Failed to send message:", error); // Show user-friendly error message alert("Failed to send message. Please try again."); });Also verify that the session token has appropriate scope and expiration for this use case.
35-36: LGTM: Consistent DOM structure improvements.The addition of
data-messageidattributes andmstackspans provides better DOM structure for thread functionality. The implementation is consistent across both message rendering scenarios.Also applies to: 57-57, 135-136, 156-156, 158-158
| await resend.emails.send({ | ||
| from: 'Clatter Auth <noreply@clatterauth.quntem.co.uk>', | ||
| to: [user.email], | ||
| subject: 'Reset Clatter Password', |
There was a problem hiding this comment.
Fix incorrect email subject.
The email subject says "Reset Clatter Password" but this function is for email verification, not password reset. This could confuse users.
- subject: 'Reset Clatter Password',
+ subject: 'Verify Your Clatter Account',📝 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.
| subject: 'Reset Clatter Password', | |
| subject: 'Verify Your Clatter Account', |
🤖 Prompt for AI Agents
In app/auth.ts at line 236, the email subject is incorrectly set to "Reset
Clatter Password" for an email verification function. Change the subject line to
something appropriate for email verification, such as "Verify Your Clatter
Email" or "Clatter Email Verification," to accurately reflect the purpose of the
email and avoid user confusion.
| console.log(url, user) | ||
| console.log(process.env.RESEND_KEY) |
There was a problem hiding this comment.
Remove sensitive information from console logs.
The console logging of URL, user data, and the Resend API key could expose sensitive information in production logs and should be removed or replaced with appropriate logging levels.
- console.log(url, user)
- console.log(process.env.RESEND_KEY)📝 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.
| console.log(url, user) | |
| console.log(process.env.RESEND_KEY) |
🤖 Prompt for AI Agents
In app/auth.ts around lines 21 to 22, remove the console.log statements that
output the URL, user data, and the Resend API key to prevent exposing sensitive
information. Replace these with more secure logging practices, such as using a
debug-level logger that can be disabled in production or removing the logs
entirely if they are not necessary.
| socket.destroy() | ||
| delete socket | ||
| socket = io() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve socket cleanup implementation.
The socket cleanup approach could be more robust. Consider checking if the socket exists before attempting to destroy it and handle potential errors.
try {
- socket.destroy()
- delete socket
- socket = io()
+ if (typeof socket !== 'undefined' && socket) {
+ socket.disconnect()
+ socket = null
+ }
+ socket = io()
} catch {📝 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.
| socket.destroy() | |
| delete socket | |
| socket = io() | |
| try { | |
| if (typeof socket !== 'undefined' && socket) { | |
| socket.disconnect() | |
| socket = null | |
| } | |
| socket = io() | |
| } catch { | |
| // handle errors | |
| } |
🤖 Prompt for AI Agents
In app/client/pages/channelthreadpage.html around lines 15 to 17, improve the
socket cleanup by first checking if the socket object exists before calling
destroy on it. Add error handling around the destroy call to catch and manage
any exceptions that might occur during cleanup. This ensures the cleanup process
is more robust and prevents runtime errors if the socket is undefined or already
closed.
| const io = new Server(server, { | ||
| cors: { | ||
| origin: "*", | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Security concern: Overly permissive CORS configuration.
Using wildcard origin ("*") in CORS configuration allows any domain to access your API, which poses a significant security risk. This configuration enables cross-origin requests from potentially malicious websites.
Apply this diff to restrict CORS to specific allowed origins:
-const io = new Server(server, {
- cors: {
- origin: "*",
- },
-})
+const io = new Server(server, {
+ cors: {
+ origin: process.env.ALLOWED_ORIGINS?.split(',') || ["http://localhost:3000"],
+ credentials: true,
+ },
+})📝 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.
| const io = new Server(server, { | |
| cors: { | |
| origin: "*", | |
| }, | |
| }) | |
| const io = new Server(server, { | |
| cors: { | |
| origin: process.env.ALLOWED_ORIGINS?.split(',') || ["http://localhost:3000"], | |
| credentials: true, | |
| }, | |
| }) |
🤖 Prompt for AI Agents
In app/server.js around lines 13 to 17, the CORS configuration uses a wildcard
origin "*" which is overly permissive and insecure. Replace the wildcard with a
list of specific allowed origins by defining an array of trusted domain URLs and
setting the cors.origin option to this array. This restricts cross-origin
requests to only those trusted domains, improving security.
| app.use( | ||
| cors({ | ||
| origin: "*", // Replace with your frontend's origin | ||
| methods: ["GET", "POST", "PUT", "DELETE"], // Specify allowed HTTP methods | ||
| credentials: true, // Allow credentials (cookies, authorization headers, etc.) | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Security concern: Overly permissive CORS configuration in Express middleware.
Similar to the Socket.IO CORS configuration, allowing all origins ("*") with credentials enabled creates a security vulnerability. This combination is particularly dangerous as it allows any website to make authenticated requests to your API.
Apply this diff to secure the CORS configuration:
-app.use(
- cors({
- origin: "*", // Replace with your frontend's origin
- methods: ["GET", "POST", "PUT", "DELETE"], // Specify allowed HTTP methods
- credentials: true, // Allow credentials (cookies, authorization headers, etc.)
- })
-);
+app.use(
+ cors({
+ origin: process.env.ALLOWED_ORIGINS?.split(',') || ["http://localhost:3000"],
+ methods: ["GET", "POST", "PUT", "DELETE"],
+ credentials: true,
+ })
+);📝 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.
| app.use( | |
| cors({ | |
| origin: "*", // Replace with your frontend's origin | |
| methods: ["GET", "POST", "PUT", "DELETE"], // Specify allowed HTTP methods | |
| credentials: true, // Allow credentials (cookies, authorization headers, etc.) | |
| }) | |
| ); | |
| app.use( | |
| cors({ | |
| origin: process.env.ALLOWED_ORIGINS?.split(',') || ["http://localhost:3000"], | |
| methods: ["GET", "POST", "PUT", "DELETE"], | |
| credentials: true, | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In app/server.js around lines 19 to 25, the CORS middleware is configured with
origin set to "*" while credentials are enabled, which is a security risk. To
fix this, replace the wildcard origin with a specific allowed origin or a list
of trusted origins matching your frontend URLs, and keep credentials enabled
only if necessary. This ensures only authorized frontends can make authenticated
requests to your API.
| if(argjson.method === "modern") { | ||
| const session = await auth.api.getSession({ | ||
| headers: new Headers({ | ||
| authorization: "Bearer " + argjson.token | ||
| }) | ||
| }); | ||
| if(argjson.parentmessageid) { | ||
| var parentid = argjson.parentmessageid | ||
| } else { | ||
| var parentid = null | ||
| } | ||
| var message = await prisma.message.create({ | ||
| data: { | ||
| content: argjson.content, | ||
| sender: session.session.userId, | ||
| parentid: argjson.room, | ||
| parentmessageid: parentid, | ||
| sendername: session.user.name | ||
| } | ||
| }) | ||
| argjson.id = message.id | ||
| } |
There was a problem hiding this comment.
Fix variable redeclaration and improve error handling.
The static analysis tool correctly identifies a variable redeclaration issue with parentid. Additionally, the code lacks proper error handling for authentication failures and database operations.
Apply this diff to fix the variable redeclaration and add error handling:
- if(argjson.method === "modern") {
- const session = await auth.api.getSession({
- headers: new Headers({
- authorization: "Bearer " + argjson.token
- })
- });
- if(argjson.parentmessageid) {
- var parentid = argjson.parentmessageid
- } else {
- var parentid = null
- }
- var message = await prisma.message.create({
- data: {
- content: argjson.content,
- sender: session.session.userId,
- parentid: argjson.room,
- parentmessageid: parentid,
- sendername: session.user.name
- }
- })
- argjson.id = message.id
- }
+ if(argjson.method === "modern") {
+ try {
+ const session = await auth.api.getSession({
+ headers: new Headers({
+ authorization: "Bearer " + argjson.token
+ })
+ });
+
+ if (!session || !session.session?.userId) {
+ socket.emit("clatter.channel.message.send.error", "Authentication failed");
+ return;
+ }
+
+ const parentMessageId = argjson.parentmessageid || null;
+
+ const message = await prisma.message.create({
+ data: {
+ content: argjson.content,
+ sender: session.session.userId,
+ parentid: argjson.room,
+ parentmessageid: parentMessageId,
+ sendername: session.user.name
+ }
+ });
+
+ argjson.id = message.id;
+ } catch (error) {
+ console.error("Error processing modern message:", error);
+ socket.emit("clatter.channel.message.send.error", "Failed to send message");
+ return;
+ }
+ }📝 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.
| if(argjson.method === "modern") { | |
| const session = await auth.api.getSession({ | |
| headers: new Headers({ | |
| authorization: "Bearer " + argjson.token | |
| }) | |
| }); | |
| if(argjson.parentmessageid) { | |
| var parentid = argjson.parentmessageid | |
| } else { | |
| var parentid = null | |
| } | |
| var message = await prisma.message.create({ | |
| data: { | |
| content: argjson.content, | |
| sender: session.session.userId, | |
| parentid: argjson.room, | |
| parentmessageid: parentid, | |
| sendername: session.user.name | |
| } | |
| }) | |
| argjson.id = message.id | |
| } | |
| if (argjson.method === "modern") { | |
| try { | |
| const session = await auth.api.getSession({ | |
| headers: new Headers({ | |
| authorization: "Bearer " + argjson.token | |
| }) | |
| }); | |
| if (!session || !session.session?.userId) { | |
| socket.emit("clatter.channel.message.send.error", "Authentication failed"); | |
| return; | |
| } | |
| const parentMessageId = argjson.parentmessageid || null; | |
| const message = await prisma.message.create({ | |
| data: { | |
| content: argjson.content, | |
| sender: session.session.userId, | |
| parentid: argjson.room, | |
| parentmessageid: parentMessageId, | |
| sendername: session.user.name | |
| } | |
| }); | |
| argjson.id = message.id; | |
| } catch (error) { | |
| console.error("Error processing modern message:", error); | |
| socket.emit("clatter.channel.message.send.error", "Failed to send message"); | |
| return; | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 266-266: Shouldn't redeclare 'parentid'. Consider to delete it or rename it.
'parentid' is defined here:
(lint/suspicious/noRedeclare)
🤖 Prompt for AI Agents
In app/server.js around lines 257 to 278, fix the variable redeclaration by
declaring 'parentid' once using 'let' before the if-else block and assign its
value conditionally. Add try-catch blocks around the authentication call and the
database create operation to handle potential errors gracefully, logging or
responding appropriately to authentication failures and database errors.
| socket.destroy() | ||
| delete socket | ||
| socket = io() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve socket cleanup and error handling.
The socket cleanup logic attempts to call socket.destroy() but this method doesn't exist on Socket.IO client instances. The proper method is socket.disconnect(). Also, consider adding more robust error handling.
Apply this diff to fix the socket cleanup:
- socket.destroy()
- delete socket
- socket = io()
+ socket.disconnect()
+ socket = io()📝 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.
| socket.destroy() | |
| delete socket | |
| socket = io() | |
| socket.disconnect() | |
| socket = io() |
🤖 Prompt for AI Agents
In app/client/pages/channelpage.html around lines 15 to 17, replace the call to
socket.destroy() with socket.disconnect() to properly close the Socket.IO client
connection. Remove the delete socket statement as it is unnecessary in
JavaScript. Additionally, wrap the socket cleanup and reinitialization code in a
try-catch block to handle any potential errors gracefully and log or handle them
appropriately.
2 participants
Summary by CodeRabbit
New Features
Improvements
Style