feat: enforce default channel validation (max 3, one per platform)#1387
feat: enforce default channel validation (max 3, one per platform)#1387Copilot wants to merge 5 commits intofeat/add-electron-platformfrom
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78e10f5c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const finalPublic = update.public !== undefined ? update.public : (existingChannel?.public ?? DEFAULT_CHANNEL_PUBLIC) | ||
| const finalIos = update.ios !== undefined ? update.ios : (existingChannel?.ios ?? DEFAULT_CHANNEL_IOS) | ||
| const finalAndroid = update.android !== undefined ? update.android : (existingChannel?.android ?? DEFAULT_CHANNEL_ANDROID) | ||
| const finalElectron = update.electron !== undefined ? update.electron : (existingChannel?.electron ?? DEFAULT_CHANNEL_ELECTRON) |
There was a problem hiding this comment.
The validation computes finalPublic/platform flags from existingChannel, but existingChannel is only fetched for the same created_by (see the query just above this block). If a teammate with app write rights updates a channel they didn’t create and omits public (common for partial updates), existingChannel is null, finalPublic falls back to false, and validatePublicChannels() is skipped. That allows updates like setting ios: true on a public channel without triggering duplicate-platform checks, so conflicting public channels can be created in collaborative apps. Consider fetching the existing channel by app_id+name regardless of creator (or otherwise ensuring finalPublic reflects the actual current row) so validation runs on all updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements backend validation rules for public (default) channels to prevent configuration conflicts and ensure proper platform distribution. The validation enforces a maximum of 3 public channels per app and restricts each platform (iOS/Android/Electron) to only one public channel.
Changes:
- Added
validatePublicChannels()function with comprehensive pre-upsert validation logic - Extended channel API to support
electronfield alongside existing iOS/Android fields - Created extensive test suite covering valid/invalid configurations, updates, and edge cases
- Documented validation rules, error messages, API usage, and migration considerations
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
supabase/functions/_backend/utils/supabase.ts |
Core validation logic checking max channels and platform uniqueness, integrated into updateOrCreateChannel |
supabase/functions/_backend/public/channel/post.ts |
Added electron field to channel creation API interface and payload |
tests/channel_default_validation.test.ts |
Comprehensive test coverage for all validation scenarios including valid configs, limit violations, platform conflicts, and private channel independence |
docs/DEFAULT_CHANNEL_VALIDATION.md |
Complete documentation of validation rules, examples, error messages, and migration notes |
Comments suppressed due to low confidence (1)
supabase/functions/_backend/utils/supabase.ts:276
- The upsert uses onConflict on 'app_id, name', but there's no visible unique constraint on these columns in the database schema. The code also checks for existing channels with app_id, name, AND created_by (line 234-236), which suggests channels might be unique per (app_id, name, created_by). If the database doesn't have a unique constraint on (app_id, name), the upsert may not work as expected and could create duplicate channels with the same name. Verify that a unique constraint exists on channels(app_id, name) in the database.
return supabaseAdmin(c)
.from('channels')
.upsert(update, { onConflict: 'app_id, name' })
.throwOnError()
| async function validatePublicChannels( | ||
| c: Context, | ||
| appId: string, | ||
| channelName: string, | ||
| isPublic: boolean, | ||
| ios: boolean, | ||
| android: boolean, | ||
| electron: boolean, | ||
| ) { | ||
| // Only validate if the channel is being set to public | ||
| if (!isPublic) { | ||
| return | ||
| } | ||
|
|
||
| // Get all existing public channels for this app (excluding the current channel being updated) | ||
| const { data: publicChannels, error } = await supabaseAdmin(c) | ||
| .from('channels') | ||
| .select('id, name, ios, android, electron') | ||
| .eq('app_id', appId) | ||
| .eq('public', true) | ||
| .neq('name', channelName) | ||
|
|
||
| if (error) { | ||
| cloudlogErr({ requestId: c.get('requestId'), message: 'Error fetching public channels', error }) | ||
| throw simpleError('db_error', 'Failed to validate public channels') | ||
| } | ||
|
|
||
| const existingPublicChannels = publicChannels || [] | ||
|
|
||
| // Rule 1: Maximum 3 public channels per app | ||
| if (existingPublicChannels.length >= 3) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Maximum 3 public channels allowed per app', | ||
| appId, | ||
| existingCount: existingPublicChannels.length, | ||
| }) | ||
| throw simpleError( | ||
| 'max_public_channels', | ||
| 'Maximum 3 public channels allowed per app. You can have one default channel for all platforms or up to three (one per platform: iOS, Android, Electron).', | ||
| ) | ||
| } | ||
|
|
||
| // Rule 2: Maximum 1 public channel per platform | ||
| // Check for platform conflicts | ||
| for (const existingChannel of existingPublicChannels) { | ||
| if (ios && existingChannel.ios) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Platform conflict: iOS', | ||
| appId, | ||
| existingChannel: existingChannel.name, | ||
| newChannel: channelName, | ||
| }) | ||
| throw simpleError( | ||
| 'duplicate_platform_ios', | ||
| `Another public channel "${existingChannel.name}" already supports iOS platform. Only one public channel per platform is allowed.`, | ||
| ) | ||
| } | ||
| if (android && existingChannel.android) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Platform conflict: Android', | ||
| appId, | ||
| existingChannel: existingChannel.name, | ||
| newChannel: channelName, | ||
| }) | ||
| throw simpleError( | ||
| 'duplicate_platform_android', | ||
| `Another public channel "${existingChannel.name}" already supports Android platform. Only one public channel per platform is allowed.`, | ||
| ) | ||
| } | ||
| if (electron && existingChannel.electron) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Platform conflict: Electron', | ||
| appId, | ||
| existingChannel: existingChannel.name, | ||
| newChannel: channelName, | ||
| }) | ||
| throw simpleError( | ||
| 'duplicate_platform_electron', | ||
| `Another public channel "${existingChannel.name}" already supports Electron platform. Only one public channel per platform is allowed.`, | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation logic does not prevent creating a public channel with all platforms disabled (ios=false, android=false, electron=false). Such a channel would be unusable by any device but would still count against the 3-channel limit and wouldn't trigger any validation errors. Consider adding validation to ensure at least one platform is enabled for public channels.
| describe('Private channels should not be affected', () => { | ||
| it('should allow multiple private channels regardless of platform', async () => { | ||
| const id9 = randomUUID() | ||
| const APPNAME9 = `com.app.private.${id9}` | ||
| await resetAndSeedAppData(APPNAME9) | ||
|
|
||
| // Create multiple private channels with same platforms - should all succeed | ||
| for (let i = 0; i < 5; i++) { | ||
| const response = await fetch(`${BASE_URL}/channel`, { | ||
| method: 'POST', | ||
| headers, | ||
| body: JSON.stringify({ | ||
| app_id: APPNAME9, | ||
| channel: `private_channel_${i}`, | ||
| public: false, | ||
| ios: true, | ||
| android: true, | ||
| electron: true, | ||
| }), | ||
| }) | ||
| expect(response.status).toBe(200) | ||
| } | ||
|
|
||
| // Verify all 5+ private channels exist (including seeded ones) | ||
| const { data: channels } = await getSupabaseClient() | ||
| .from('channels') | ||
| .select('*') | ||
| .eq('app_id', APPNAME9) | ||
| .eq('public', false) | ||
|
|
||
| expect(channels?.length).toBeGreaterThanOrEqual(5) | ||
|
|
||
| await resetAppData(APPNAME9) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Missing test coverage for converting a private channel to public when it would violate validation rules. Consider adding a test that creates 3 public channels, then creates a 4th private channel, and attempts to update that private channel to public (which should fail with max_public_channels error).
| // Get all existing public channels for this app (excluding the current channel being updated) | ||
| const { data: publicChannels, error } = await supabaseAdmin(c) | ||
| .from('channels') | ||
| .select('id, name, ios, android, electron') | ||
| .eq('app_id', appId) | ||
| .eq('public', true) | ||
| .neq('name', channelName) | ||
|
|
||
| if (error) { | ||
| cloudlogErr({ requestId: c.get('requestId'), message: 'Error fetching public channels', error }) | ||
| throw simpleError('db_error', 'Failed to validate public channels') | ||
| } | ||
|
|
||
| const existingPublicChannels = publicChannels || [] | ||
|
|
||
| // Rule 1: Maximum 3 public channels per app | ||
| if (existingPublicChannels.length >= 3) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Maximum 3 public channels allowed per app', | ||
| appId, | ||
| existingCount: existingPublicChannels.length, | ||
| }) | ||
| throw simpleError( | ||
| 'max_public_channels', | ||
| 'Maximum 3 public channels allowed per app. You can have one default channel for all platforms or up to three (one per platform: iOS, Android, Electron).', | ||
| ) | ||
| } |
There was a problem hiding this comment.
Potential race condition: If two concurrent requests attempt to create public channels that would together violate the validation rules, both could pass validation (each seeing only 2 existing channels) and both get created, resulting in 4 public channels. Consider adding a database-level constraint (unique partial index or check constraint) to enforce these rules at the database level, or using transaction-level row locking.
| @@ -0,0 +1,447 @@ | |||
| import { randomUUID } from 'node:crypto' | |||
| import { afterAll, beforeAll, describe, expect, it } from 'vitest' | |||
| import { BASE_URL, headers, resetAndSeedAppData, resetAppData, getSupabaseClient, ORG_ID, USER_ID } from './test-utils.ts' | |||
There was a problem hiding this comment.
Unused imports ORG_ID, USER_ID.
| import { BASE_URL, headers, resetAndSeedAppData, resetAppData, getSupabaseClient, ORG_ID, USER_ID } from './test-utils.ts' | |
| import { BASE_URL, headers, resetAndSeedAppData, resetAppData, getSupabaseClient } from './test-utils.ts' |
Summary
Implements backend validation to prevent public channel conflicts: maximum 3 public channels per app, and only one public channel per platform (iOS/Android/Electron). Users can now have either one universal default channel OR up to three platform-specific channels without ambiguity.
Implementation:
utils/supabase.ts):validatePublicChannels()runs pre-upsert, checks channel count and platform uniqueness, throws typed errors (max_public_channels,duplicate_platform_{ios|android|electron})public/channel/post.ts): Addedelectronfield support to match iOS/Android patternchannel_default_validation.test.ts): Valid configs (single multi-platform, three single-platform, mixed), invalid configs (4+ channels, platform duplicates), update scenariosdocs/DEFAULT_CHANNEL_VALIDATION.md): Rules reference, error messages, migration guidanceExample validation:
Private channels unaffected. Validation uses final channel state (merges update with existing values).
Test plan
bun test:backendto verify validation test suitemax_public_channelserrorduplicate_platform_*errorScreenshots
N/A - Backend validation only
Checklist
bun run lint:backend && bun run lint.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.