fix(messaging): let members create channels, name DMs after participants - #97
Merged
Conversation
Two defects confirmed against a running instance with a non-admin user. Members could not create a channel. POST /messaging/channels was gated on messaging:manage, which defaults to admin only, so every member got a 403 and the sidebar's "New channel" button could never succeed. Split channel creation into its own messaging:create_channel permission granted to admin and member, leaving rename/archive on the admin-only manage gate. Adding a permission to a ModuleDefinition only affects new workspaces, since seedWorkspaceRoles runs once when a workspace's Member role is created. Added a migration backfilling create_channel to every role that already holds messaging:send. DM channels are all stored as name='dm' with the display name expected to be built from participants, but GET /messaging/channels returned no members, so every DM rendered as the literal string "dm". The list endpoint now attaches members for dm/group_dm rows, and a shared channelDisplayName() helper derives the label from the other participants for the sidebar, channel header, and composer placeholder. Covers both with tests: permission gates asserted off the router, and channelDisplayName across regular channels, DMs, group DMs, and the not-yet-loaded and only-self fallbacks.
The api service exposed 3001 but published no host port, so a locally-run `pnpm dev` web server proxied /api/* to a dead localhost:3001 through the Next.js rewrite. Every API call failed at the proxy and the UI rendered an empty shell with no error, which reads as an application bug. Publish on loopback only.
There was a problem hiding this comment.
🟡 Not ready to approve
There are a few concrete correctness/rollback-safety issues (DM naming when current user id is not yet known, missing regression coverage for that case, and overly-broad down() deletion in the migration) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes two messaging bugs (member channel creation + DM naming) and unblocks local development by publishing the API service port so the web dev server can reach it via Next.js rewrites.
Changes:
- Split channel creation into a new
messaging:create_channelpermission granted to members, while keeping admin-only management onmessaging:manage. - Add DM/member data to the channels list response for
dm/group_dmand introduce a sharedchannelDisplayName()helper for consistent DM naming across the UI. - Publish the API container’s port on loopback in
docker-compose.ymlto restore local/api/*proxying.
File summaries
| File | Description |
|---|---|
| packages/modules/src/messaging/index.ts | Adds messaging:create_channel permission and documents the intent. |
| packages/db/migrations/20260803_001_messaging_create_channel_permission.ts | Backfills the new permission to existing roles that already have messaging:send. |
| docker-compose.yml | Publishes api on 127.0.0.1:3001 for local dev proxy/WS connectivity. |
| apps/web/modules/messaging/lib/dm-name.ts | Introduces channelDisplayName() to derive DM/group DM display names from participants. |
| apps/web/modules/messaging/lib/dm-name.test.ts | Adds unit tests for channelDisplayName(). |
| apps/web/modules/messaging/components/ChannelView.tsx | Uses channelDisplayName() in the header and composer placeholder. |
| apps/web/modules/messaging/components/ChannelSidebar.tsx | Uses channelDisplayName() for DM rows and extends the row type to include members. |
| apps/api/src/routes/messaging/channels.ts | Attaches DM members in the list endpoint and gates channel creation on messaging:create_channel. |
| apps/api/src/tests/messaging-channels-permissions.test.ts | Adds route-gating and module permission definition tests for the new permission split. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+23
to
+26
| const isDm = channel.type === 'dm' || channel.type === 'group_dm'; | ||
| if (!isDm) return channel.name; | ||
|
|
||
| const others = (channel.members ?? []).filter(m => m.user_id !== currentUserId); |
Comment on lines
+21
to
+25
| export async function down(db: Kysely<unknown>): Promise<void> { | ||
| await sql` | ||
| delete from role_permissions where permission = 'messaging:create_channel' | ||
| `.execute(db); | ||
| } |
Comment on lines
+45
to
+47
| it('falls back to a readable label when members are not loaded', () => { | ||
| expect(channelDisplayName({ name: 'dm', type: 'dm' }, ME)).toBe('Direct message'); | ||
| }); |
The RBAC3 refactor dropped users.role, but tasks-unified.ts still read it, so
`tsc` failed and development did not compile:
src/routes/tasks-unified.ts(88,40): error TS2339: Property 'role' does not
exist on type '{ id: string; workspace_id: string; ... }'
isAdmin is already destructured from the authenticated request one line above
and was simply unused. This mirrors the fix already on main (ea4922f).
Unrelated to the messaging changes in this PR, but development's build, lint
and test jobs all fail without it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Reported as "the messaging module has zero working features — private messages don't work and channel messages aren't visible."
The messaging module itself was largely fine. Most of the symptom was the local dev environment failing in a way that renders an empty page with no error at all. Two genuine bugs were found underneath, both confirmed against a running instance with a real non-admin user.
This is the
development-targeted counterpart to #96. See the note at the bottom on why the two PRs differ.Bug 1 — members could not create channels
POST /messaging/channelswas gated onmessaging:manage, which defaults to admin only. Every member hit403 FORBIDDEN, so the sidebar's "New channel" button could never succeed.Split channel creation into its own
messaging:create_channelpermission granted to admin and member. Rename/archive stay on the admin-onlymanagegate.Adding a permission to a
ModuleDefinitiononly reaches new workspaces —seedWorkspaceRolesruns once, when a workspace's Member role is created. Added a migration backfillingcreate_channelto every role already holdingmessaging:send. Roles withgrants_allare unaffected; they short-circuit to superuser and never consultrole_permissions.Bug 2 — every DM rendered as the literal string "dm"
DM channels are stored as
name='dm', with the display name meant to be built from participants — butGET /messaging/channelsreturned no members, so the frontend had nothing to build from. Every DM row in the sidebar looked identical.The list endpoint now attaches
membersfordm/group_dmrows only. A sharedchannelDisplayName()helper derives the label from the other participants, used by the sidebar row, channel header, and composer placeholder.Dev environment
The
apiservice exposed 3001 but published no host port, so a locally-runpnpm devweb server proxied/api/*to a deadlocalhost:3001. Every call failed at the proxy and the UI rendered an empty shell with no error — this is what made the original report look like a total module failure. Now published on loopback.Verification
Seeded a second workspace user and drove the running app as a non-admin:
POST /channels→ 201, channel appears in the sidebarTest results — please read
13 new tests (6 API, 7 web), all passing. Web suite fully green: 43/43.
The API suite on this branch reports 25 failed / 323 passed. Those 25 failures are pre-existing on
developmentand unrelated to this change — they are in contacts, pipeline-items, pipelines, projects, and soft-delete-filters. Measured against a cleanorigin/developmentcheckout:developmentDelta is exactly +6 passing tests, no new failures.
development's broken suite is worth a separate look.Relationship to #96
#96 targets
mainand additionally contains a fix for@vencore/configleakingfsinto the browser bundle. That fix does not apply here:AppearanceControls.tsx,packages/config/src/palette.ts, andpresets.tsexist only onmain, and the bug only manifests because of them.origin/mainalready containsorigin/developmentand is 50 commits ahead, so the branches have diverged. Merging both PRs is safe — the messaging commits are identical in content.Known issue, not addressed here
The dev database's migration ledger is corrupted independently of this change:
kysely_migrationrecords20260722/20260724/20260725entries that do not exist inpackages/db/migrationson either branch, sopnpm db:migraterefuses to run. The new migration was validated by applying its SQL directly; it is idempotent and will no-op once the ledger is reconciled.Still open
Not fixed here:
typing.startbroadcasts workspace-wide instead of per-channel;GET /dmsis dead code; noisErrorbranch anywhere, so API failures render as empty UI with no feedback; the JWT is passed in the WebSocket query string where the cookie already works.