feat: add Slack-like channels with sidebar UI and channel-scoped messaging#130
feat: add Slack-like channels with sidebar UI and channel-scoped messaging#130
Conversation
…aging Add a Channel feature where each message belongs to a single channel and channels can have multiple members. All users are auto-joined to #general. - Add channels and channel_members tables with migration (seeds #general, backfills existing messages) - Add channel CRUD API routes (list, create, join, leave) with 14 tests - Update messages API to require channelId query parameter - Switch WebSocket protocol from plain text to JSON with channel-scoped broadcast (only channel members receive messages) - Add dark-themed Slack-like UI with channel sidebar, create channel modal, join/leave functionality - Auto-join users to #general on page visit - Update all existing tests for new HTML structure and API changes
There was a problem hiding this comment.
Pull request overview
This PR introduces Slack-like channels to the MLack chat app, making messages channel-scoped across the database, HTTP APIs, WebSocket protocol, and UI.
Changes:
- Add
channelsandchannel_memberstables; makemessagesbelong to a channel viachannel_idand seed/backfill#general. - Add channel CRUD-ish endpoints (list/create/join/leave) and require
channelIdforGET /api/messages. - Update UI to a sidebar-based channel experience and switch WS traffic to JSON with channel-scoped broadcast.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| hono/types.ts | Adds a shared Channel type. |
| hono/static/ChatPage.ts | Implements channel sidebar behavior, channel-scoped message loading, and JSON WS send/receive. |
| hono/routes/ws.ts | Switches WS protocol to JSON and scopes broadcast by channel membership. |
| hono/routes/ws.test.ts | Updates WS tests for Map-based client tracking and membership query mocking. |
| hono/routes/messages.ts | Requires channelId query parameter and filters messages by channel. |
| hono/routes/messages.test.ts | Updates/extends tests for the new channelId requirement. |
| hono/routes/index.tsx | Auto-joins authenticated users into #general on page load. |
| hono/routes/index.test.tsx | Updates root page assertions for the new channel header structure. |
| hono/routes/channels.ts | Adds channel list/create/join/leave endpoints. |
| hono/routes/channels.test.ts | Adds test coverage for channel endpoints. |
| hono/db/schema.ts | Adds new tables and messages.channel_id FK. |
| hono/db/migrations/0002_narrow_maggott.sql | Creates channel tables, seeds general, and backfills existing messages. |
| hono/db/migrations/meta/_journal.json | Registers the new migration. |
| hono/db/migrations/meta/0002_snapshot.json | Captures the new schema snapshot for Drizzle migrations. |
| hono/components/ChatPage.tsx | Reworks chat layout into Slack-like sidebar + main area and adds create-channel modal markup. |
| hono/components/ChatPage.test.ts | Updates component tests for the new structure/classnames. |
| hono/components/ChatPage.css | Adds new dark Slack-like styling for sidebar/main/modal. |
| hono/app.tsx | Registers the new channels route and updates WS client tracking to Map. |
Comments suppressed due to low confidence (2)
hono/routes/channels.ts:53
- Channel creation does two separate inserts (into
channelsand thenchannel_members) without a transaction, so a failure on the second insert can leave a channel that the creator is not a member of. Wrap these writes in a single DB transaction (and consider validatingnamelength <= 255 to avoid DB errors) to keep state consistent.
const [created] = await db
.insert(channels)
.values({ name, createdByEmail: user.email || "unknown" })
.returning();
await db.insert(channelMembers).values({ channelId: created.id, userEmail: user.email || "unknown" });
hono/components/ChatPage.tsx:66
- The create-channel modal markup lacks dialog accessibility semantics (e.g.,
role="dialog",aria-modal="true", and an accessible label viaaria-labelledby). Without these, screen readers may not announce the modal properly or confine navigation; add the appropriate ARIA attributes and ensure focus management when opening/closing.
<div id="createChannelModal" className="modal hidden">
<div className="modal-content">
<h3>Create Channel</h3>
<input type="text" id="newChannelName" placeholder="channel-name" />
<div className="modal-actions">
<button type="button" id="cancelCreateChannel">Cancel</button>
<button type="button" id="confirmCreateChannel">Create</button>
</div>
| export const messages = pgTable("messages", { | ||
| id: serial("id").primaryKey(), | ||
| content: text("content").notNull(), | ||
| userEmail: varchar("user_email", { length: 255 }).notNull(), | ||
| userName: varchar("user_name", { length: 255 }), | ||
| channelId: integer("channel_id") | ||
| .notNull() | ||
| .references(() => channels.id), | ||
| createdAt: timestamp("created_at").defaultNow(), | ||
| }); |
There was a problem hiding this comment.
Queries now filter messages by channel_id and order by created_at (e.g., /api/messages?channelId=…). There’s no index to support WHERE channel_id = ? ORDER BY created_at DESC LIMIT 100, which will lead to sequential scans as the table grows; consider adding an index on (channel_id, created_at) (and/or at least channel_id).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const existingMembership = await db | ||
| .select() | ||
| .from(channelMembers) | ||
| .where( | ||
| and(eq(channelMembers.channelId, generalChannel.id), eq(channelMembers.userEmail, user.email || "unknown")), | ||
| ); | ||
|
|
||
| if (existingMembership.length === 0) { | ||
| await db.insert(channelMembers).values({ | ||
| channelId: generalChannel.id, | ||
| userEmail: user.email || "unknown", | ||
| }); |
There was a problem hiding this comment.
Using user.email || "unknown" when creating/checking membership can merge multiple users into the same "unknown" identity if email is missing, which can leak messages/memberships across accounts. Prefer treating missing user.email as an error (e.g., redirect to login or return 500) and avoid writing a shared placeholder email to the DB.
| const name = body.name?.trim(); | ||
|
|
There was a problem hiding this comment.
const name = body.name?.trim(); will throw if body.name is present but not a string (e.g., number/object), since optional chaining here only guards null/undefined. Validate the request body type (ensure typeof body.name === "string") before calling .trim() and return 400 on invalid input.
This issue also appears on line 47 of the same file.
| const name = body.name?.trim(); | |
| if (!body || typeof body !== "object" || body === null || typeof (body as any).name !== "string") { | |
| return c.json({ error: "Channel name must be a string" }, 400); | |
| } | |
| const name = (body as { name: string }).name.trim(); |
There was a problem hiding this comment.
@copilot Shouldn't this be covered by type checking instead?
| const channelIdParam = c.req.query("channelId"); | ||
| if (!channelIdParam) { | ||
| return c.json({ error: "channelId query parameter is required" }, 400); | ||
| } | ||
|
|
||
| const channelId = Number(channelIdParam); | ||
| if (Number.isNaN(channelId)) { | ||
| return c.json({ error: "Invalid channelId" }, 400); | ||
| } | ||
|
|
||
| const allMessages = await db | ||
| .select() | ||
| .from(messages) | ||
| .where(eq(messages.channelId, channelId)) | ||
| .orderBy(desc(messages.createdAt)) | ||
| .limit(100); // Limit to last 100 messages | ||
| .limit(100); |
There was a problem hiding this comment.
This endpoint returns messages for any channelId without verifying the requesting user is a member of that channel. That allows authenticated users to read message history from channels they haven't joined; add an authorization check against channel_members (and return 403/404) before querying messages.
| channelsRoute.get("/api/channels", async (c) => { | ||
| try { | ||
| const session = c.get("session"); | ||
| const user = session.get("user") as User | undefined; | ||
|
|
||
| if (!user) { | ||
| return c.json({ error: "Unauthorized" }, 401); | ||
| } | ||
|
|
There was a problem hiding this comment.
PR description says GET /api/channels is public, but this handler returns 401 when there is no session user. Either adjust the implementation to match the described public behavior, or update the PR description/API contract to reflect that authentication is required.
| let parsed: { type: string; channelId: number; content: string }; | ||
| try { | ||
| parsed = JSON.parse(rawStr); | ||
| } catch { | ||
| return; | ||
| } | ||
|
|
||
| if (parsed.type !== "message" || !parsed.channelId || !parsed.content?.trim()) { | ||
| return; | ||
| } | ||
|
|
||
| // Get user info from session | ||
| const session = c.get("session"); | ||
| const user = session.get("user") as User | undefined; | ||
| const channelId = parsed.channelId; | ||
| const content = parsed.content.trim(); |
There was a problem hiding this comment.
Incoming WS payload validation is too loose: channelId is assumed to be a number, but JSON could provide a string/float/NaN and it will flow into DB insert/query. Add strict runtime validation (e.g., typeof channelId === 'number' && Number.isInteger(channelId)) and ensure content is a string before using them.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Initial plan * feat: add composite index on (channel_id, created_at) to messages table Agent-Logs-Url: https://github.com/mahata/mlack/sessions/d188e657-de69-47e9-9277-0c7227faaffd Co-authored-by: mahata <23497+mahata@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mahata <23497+mahata@users.noreply.github.com>
# Conflicts: # hono/app.tsx # hono/components/ChatPage.tsx # hono/db/migrations/meta/0002_snapshot.json # hono/db/migrations/meta/_journal.json # hono/db/schema.ts # hono/routes/messages.test.ts # hono/routes/messages.ts # hono/routes/ws.test.ts # hono/routes/ws.ts
* Initial plan * fix: add h1 Hello, world! heading to ChatPage for e2e test Agent-Logs-Url: https://github.com/mahata/mlack/sessions/00133b70-6800-4b81-9ad8-fb4b19a6e07c Co-authored-by: mahata <23497+mahata@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mahata <23497+mahata@users.noreply.github.com>
Summary
#generalchannel on first visit;#generalcannot be leftChanges
Database
channelstable (id, name, created_by_email, created_at) with unique name constraintchannel_memberstable (id, channel_id, user_email, joined_at) with unique (channel_id, user_email) constraintchannel_id(NOT NULL, FK) column tomessagestable#generaland backfills all existing messages into itAPI
GET /api/channels— list all channels (public)POST /api/channels— create a new channel (creator auto-joins)POST /api/channels/:id/join— join a channelPOST /api/channels/:id/leave— leave a channel (blocked for #general)GET /api/messages?channelId=N— now requireschannelIdquery parameterWebSocket
{ "type": "message", "channelId": 1, "content": "Hello!" }{ "type": "message", "channelId": 1, "content": "Hello!", "userName": "...", "userEmail": "..." }clientschanged fromSet<WSContext>toMap<WSContext, { userEmail }>for channel-scoped deliveryUI
Tests
channels.test.ts)messages.test.tsforchannelIdrequirement (4 tests)ws.test.tsfor Map-based client trackingChatPage.test.tsandindex.test.tsxfor new HTML structureDeployment Note
Run
pnpm db:migrateto apply the new migration before deploying.