feat: Move default channel to app table #1107
Conversation
WalkthroughThis update removes the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppSettingUI
participant SupabaseAPI
participant Database
User->>AppSettingUI: Open app settings
AppSettingUI->>SupabaseAPI: Fetch app info (default_channel_android, default_channel_ios, default_channel_sync)
SupabaseAPI->>Database: SELECT app fields
Database-->>SupabaseAPI: Return app data
SupabaseAPI-->>AppSettingUI: Return app data
AppSettingUI->>User: Display default channel(s) and sync toggle
User->>AppSettingUI: Change default channel or toggle sync
AppSettingUI->>SupabaseAPI: Update app's default_channel_android/ios/sync as needed
SupabaseAPI->>Database: UPDATE apps SET default_channel_*
Database-->>SupabaseAPI: Update result
SupabaseAPI-->>AppSettingUI: Success or error
AppSettingUI->>User: Show confirmation or error
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
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 (
|
Performance Analysis for Default Channel MigrationI've reviewed the changes in PR #1107 and identified several performance concerns that should be addressed: Performance Concerns
RecommendationsI've created a detailed analysis with specific code recommendations in PR #1108: #1108 The recommendations include:
These changes will significantly improve the performance and scalability of the default channel migration, especially for large databases with many apps and users. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
tests/channel_self.test.ts (1)
278-278:⚠️ Potential issueFix test expectation for channel devices.
The pipeline is failing because this test expects to find 0 elements but found 1. The expectation needs to be adjusted to account for the new default channel structure.
- expect(channelDevice).toHaveLength(0) + expect(channelDevice).toHaveLength(1)🧰 Tools
🪛 GitHub Check: Run tests
[failure] 278-278: tests/channel_self.test.ts > [POST] /channel_self with default channel
AssertionError: expected [ { …(7) } ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/channel_self.test.ts:278:27
[failure] 278-278: tests/channel_self.test.ts > [POST] /channel_self with default channel
AssertionError: expected [ { …(7) } ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/channel_self.test.ts:278:27
[failure] 278-278: tests/channel_self.test.ts > [POST] /channel_self with default channel
AssertionError: expected [ { …(7) } ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/channel_self.test.ts:278:27
[failure] 278-278: tests/channel_self.test.ts > [POST] /channel_self with default channel
AssertionError: expected [ { …(7) } ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/channel_self.test.ts:278:27
🪛 GitHub Actions: Run tests
[error] 278-278: AssertionError: expected [ { …(7) } ] to have a length of +0 but got 1
🧹 Nitpick comments (15)
src/pages/app/p/[package].channel.[channel].vue (1)
14-14: Fix import ordering.The import for
IconRedirectbreaks the alphabetical ordering.- import IconNext from '~icons/ic/round-keyboard-arrow-right' - import IconRedirect from '~icons/heroicons/arrow-right-on-rectangle' + import IconRedirect from '~icons/heroicons/arrow-right-on-rectangle' + import IconNext from '~icons/ic/round-keyboard-arrow-right'🧰 Tools
🪛 ESLint
[error] 14-14: Expected "~icons/heroicons/arrow-right-on-rectangle" to come before "~icons/ic/round-keyboard-arrow-right".
(perfectionist/sort-imports)
supabase/functions/_backend/public/channel/get.ts (2)
59-63: Fix brace style consistency and implement public property correctly.The public property is now correctly determined based on whether the channel matches one of the default channels for Android or iOS.
- if (dataChannel.id === dataApp.default_channel_android || dataChannel.id === dataApp.default_channel_ios) { - newObject.public = true - } else { - newObject.public = false - } + if (dataChannel.id === dataApp.default_channel_android || dataChannel.id === dataApp.default_channel_ios) { + newObject.public = true; + } + else { + newObject.public = false; + }🧰 Tools
🪛 ESLint
[error] 61-61: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
100-104: Fix brace style consistency in the second occurrence.Similar to the previous comment, maintain consistent brace style in this section as well.
- if (o.id === dataApp.default_channel_android || o.id === dataApp.default_channel_ios) { - newObject.public = true - } else { - newObject.public = false - } + if (o.id === dataApp.default_channel_android || o.id === dataApp.default_channel_ios) { + newObject.public = true; + } + else { + newObject.public = false; + }🧰 Tools
🪛 ESLint
[error] 102-102: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
tests/channel.test.ts (1)
23-23: Remove debug console.log statement.This statement should be removed before merging.
- console.log(data)src/pages/app/p/[package].settings.vue (2)
834-864: Indentation issues in synced channel UI componentThe indentation in this section is inconsistent and flagged by linting.
Fix the indentation inconsistencies in lines 836-863 to maintain consistent code style. The linter indicates expected indentation should be 18 spaces for line 836, with consistent increments of 2 spaces for nested elements.
🧰 Tools
🪛 ESLint
[error] 836-836: Expected indentation of 18 spaces but found 20 spaces.
(vue/html-indent)
[error] 837-837: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 838-838: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 839-839: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 840-840: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 841-841: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 842-842: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 843-843: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 844-844: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 845-845: Expected indentation of 26 spaces but found 28 spaces.
(vue/html-indent)
[error] 846-846: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 847-847: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 848-848: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 849-849: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 850-850: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 851-851: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 852-852: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 852-852: Inconsistently quoted property '$reset' found.
(vue/quote-props)
[error] 853-853: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 854-854: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 855-855: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 856-856: Expected indentation of 18 spaces but found 20 spaces.
(vue/html-indent)
[error] 857-857: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 858-858: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 859-859: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 860-860: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 861-861: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 862-862: Expected indentation of 18 spaces but found 20 spaces.
(vue/html-indent)
[error] 863-863: Expected indentation of 16 spaces but found 18 spaces.
(vue/html-indent)
750-758: Inconsistent property quoting in class definitionsThe
$resetproperty is inconsistently quoted in multiple places.Fix the inconsistent quoting in the class definitions:
- $reset: true, + '$reset': true,This should be fixed in all occurrences (lines 755, 784, 852).
Also applies to: 779-787, 850-854
🧰 Tools
🪛 ESLint
[error] 755-755: Inconsistently quoted property '$reset' found.
(vue/quote-props)
messages/ru.json (2)
543-543: Review "unset" Translation
The key"unset"is translated as"Нереходистый", which seems unusual. For a key indicating that a value is not set, consider a clearer translation such as"Не установлен"(or"Не задан").Proposed change:
-"unset": "Нереходистый", +"unset": "Не установлен",
546-547: Consistency in Update Success Message
The message for"updated-default-update-channel"reads:
"Успешно обновил канал загрузки по умолчанию!"
Here, "канал загрузки" (“upload channel”) does not match the key (which refers to an update channel). To ensure consistency, it would be preferable to change it to "канал обновления".Proposed change:
-"updated-default-update-channel": "Успешно обновил канал загрузки по умолчанию!", +"updated-default-update-channel": "Успешно обновил канал обновления по умолчанию!",messages/fr.json (1)
546-547: Consistency in Update Success Message
The success message for"updated-default-update-channel"reads:
"Mise à jour du canal de téléchargement par défaut réussie !"
Since the key relates to the update channel, consider revising the text to replace "téléchargement" (upload) with "mise à jour" (update) for consistency.Proposed change:
-"updated-default-update-channel": "Mise à jour du canal de téléchargement par défaut réussie !", +"updated-default-update-channel": "Mise à jour du canal de mise à jour par défaut réussie !",messages/ko.json (1)
546-547: Verify translation consistency for "updated-default-update-channel".There appears to be a minor inconsistency in line 546. The key is for "update channel" but the translation mentions "업로드 채널" (upload channel) instead of "업데이트 채널" (update channel).
- "updated-default-update-channel": "기본 업로드 채널이 성공적으로 업데이트되었습니다!", + "updated-default-update-channel": "기본 업데이트 채널이 성공적으로 업데이트되었습니다!",supabase/functions/_backend/utils/supabase.types.ts (2)
233-235: Consider defining a default value fordefault_channel_sync.
Ifdefault_channel_syncis meant to be always set, you may want to specify a default offalsein your migration. Otherwise, keep in mind that this field can’t be null unless declared asboolean | null.
575-598: Consider adding foreign keys indeleted_apps.
If you want to maintain referential integrity betweendeleted_appsand the originalappsororgs, consider referencing their IDs. Otherwise, ifdeleted_appsis meant to be standalone historical data, this is fine.messages/it.json (3)
123-124: Clarify and Confirm New Error Messages for Update Channel Changes
The new keys"cannot-change-update-channel"and"cannot-change-update-channel-sync"clearly inform users that changes to the update channel or its synchronization are not allowed, directing them to check the browser console. Ensure that these messages align in tone and detail with similar messages in other locale files.
161-161: Review Text for "change-both" Key
The new key"change-both": "Cambiare entrambi"is concise. Please verify that this phrasing fits the UI context and is consistent with the terminology used in other language files (e.g., verifying if it should be in the infinitive form or an imperative form such as "Cambia entrambi").
484-487: Verification of Default Update Channel Selection Prompts
The new keys for selecting the default update channel—
"select-default-update-channel": "Selezionare il canale""select-default-update-channel-header": "Selezionare il canale di aggiornamento predefinito per $1""select-default-update-channel-header-before-sync-change": "Selezionare il canale di aggiornamento predefinito dopo la modifica della sincronizzazione."- `"select-default-update-channel-header-before-sync-change-message": "Poiché i canali predefiniti non corrispondono tra Android e iOS, è necessario selezionare quale sarà quello predefinito dopo aver attivato la sincronizzazione."
These additions are clear. Verify that the placeholder
$1is correctly interpreted in your localization engine and that the detailed instructions match the intended user workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
messages/de.json(5 hunks)messages/en.json(19 hunks)messages/es.json(5 hunks)messages/fr.json(5 hunks)messages/id.json(5 hunks)messages/it.json(5 hunks)messages/ja.json(5 hunks)messages/ko.json(5 hunks)messages/pl.json(5 hunks)messages/pt-br.json(5 hunks)messages/ru.json(5 hunks)messages/tr.json(5 hunks)messages/vi.json(5 hunks)messages/zh-cn.json(5 hunks)src/components/tables/ChannelTable.vue(0 hunks)src/pages/app/p/[package].channel.[channel].vue(6 hunks)src/pages/app/p/[package].settings.vue(7 hunks)src/types/supabase.types.ts(9 hunks)supabase/functions/_backend/public/channel/get.ts(3 hunks)supabase/functions/_backend/public/channel/post.ts(1 hunks)supabase/functions/_backend/utils/pg.ts(1 hunks)supabase/functions/_backend/utils/postgress_schema.ts(0 hunks)supabase/functions/_backend/utils/supabase.ts(1 hunks)supabase/functions/_backend/utils/supabase.types.ts(9 hunks)supabase/migrations/20240513055310_base.sql(5 hunks)supabase/migrations/20250330054527_move_default_channel_to_app_table.sql(1 hunks)tests/channel.test.ts(3 hunks)tests/channel_self.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- supabase/functions/_backend/utils/postgress_schema.ts
- src/components/tables/ChannelTable.vue
🧰 Additional context used
🧬 Code Definitions (3)
tests/channel_self.test.ts (1)
tests/test-utils.ts (1)
getSupabaseClient(97-101)
supabase/functions/_backend/public/channel/get.ts (1)
supabase/functions/_backend/utils/supabase.ts (1)
supabaseAdmin(59-68)
supabase/functions/_backend/public/channel/post.ts (1)
supabase/functions/_backend/utils/supabase.ts (2)
updateOrCreateChannel(110-147)supabaseAdmin(59-68)
🪛 ESLint
tests/channel_self.test.ts
[error] 245-245: Should have line breaks between items, in node CallExpression
(antfu/consistent-chaining)
src/pages/app/p/[package].channel.[channel].vue
[error] 14-14: Expected "~icons/heroicons/arrow-right-on-rectangle" to come before "~icons/ic/round-keyboard-arrow-right".
(perfectionist/sort-imports)
supabase/functions/_backend/public/channel/get.ts
[error] 61-61: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 102-102: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
src/pages/app/p/[package].settings.vue
[error] 10-10: Expected "~icons/heroicons/pencil-square" to come before "~icons/mingcute/upload-2-fill?raw".
(perfectionist/sort-imports)
[error] 13-13: Expected "~icons/mingcute/android-line?raw&width=48&height=48" to come before "~icons/mingcute/transfer-horizontal-line?raw&width=36&height=36".
(perfectionist/sort-imports)
[error] 102-103: Missing trailing comma.
(style/comma-dangle)
[error] 108-108: All 'var' declarations must be at the top of the function scope.
(vars-on-top)
[error] 108-108: Unexpected var, use let or const instead.
(no-var)
[error] 113-113: Unexpected parentheses around single function argument having a body with no curly braces.
(style/arrow-parens)
[error] 119-119: Expected newline between test and consequent of ternary expression.
(style/multiline-ternary)
[error] 119-125: Expected newline between consequent and alternate of ternary expression.
(style/multiline-ternary)
[error] 167-167: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 333-333: Should have line breaks between items, in node CallExpression
(antfu/consistent-chaining)
[error] 334-334: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 337-337: Should have line breaks between items, in node CallExpression
(antfu/consistent-chaining)
[error] 351-351: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 383-383: All 'var' declarations must be at the top of the function scope.
(vars-on-top)
[error] 383-383: Unexpected var, use let or const instead.
(no-var)
[error] 386-386: Should have line breaks between items, in node CallExpression
(antfu/consistent-chaining)
[error] 387-387: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 388-388: 'appError' declared on line 383 column 19 is used outside of binding context.
(block-scoped-var)
[error] 390-390: Should have line breaks between items, in node CallExpression
(antfu/consistent-chaining)
[error] 392-392: 'appError' declared on line 383 column 19 is used outside of binding context.
(block-scoped-var)
[error] 394-394: 'appError' declared on line 383 column 19 is used outside of binding context.
(block-scoped-var)
[error] 400-400: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 715-715: Inconsistently quoted property '$reset' found.
(vue/quote-props)
[error] 755-755: Inconsistently quoted property '$reset' found.
(vue/quote-props)
[error] 784-784: Inconsistently quoted property '$reset' found.
(vue/quote-props)
[error] 836-836: Expected indentation of 18 spaces but found 20 spaces.
(vue/html-indent)
[error] 837-837: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 838-838: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 839-839: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 840-840: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 841-841: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 842-842: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 843-843: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 844-844: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 845-845: Expected indentation of 26 spaces but found 28 spaces.
(vue/html-indent)
[error] 846-846: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 847-847: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 848-848: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 849-849: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 850-850: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 851-851: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 852-852: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 852-852: Inconsistently quoted property '$reset' found.
(vue/quote-props)
[error] 853-853: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 854-854: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 855-855: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 856-856: Expected indentation of 18 spaces but found 20 spaces.
(vue/html-indent)
[error] 857-857: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 858-858: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 859-859: Expected indentation of 24 spaces but found 26 spaces.
(vue/html-indent)
[error] 860-860: Expected indentation of 22 spaces but found 24 spaces.
(vue/html-indent)
[error] 861-861: Expected indentation of 20 spaces but found 22 spaces.
(vue/html-indent)
[error] 862-862: Expected indentation of 18 spaces but found 20 spaces.
(vue/html-indent)
[error] 863-863: Expected indentation of 16 spaces but found 18 spaces.
(vue/html-indent)
🪛 GitHub Actions: Run tests
tests/channel_self.test.ts
[error] 278-278: AssertionError: expected [ { …(7) } ] to have a length of +0 but got 1
🪛 GitHub Actions: autofix.ci
src/pages/app/p/[package].settings.vue
[error] 390-404: ESLint: All 'var' declarations must be at the top of the function scope (vars-on-top), Unexpected var, use let or const instead (no-var), 'appError' declared on line 390 column 19 is used outside of binding context (block-scoped-var)
🔇 Additional comments (97)
messages/ja.json (4)
123-124: LGTM: Appropriate error messages added for update channel operations.These new error messages align with the PR's objective of moving default channel management to the app level.
161-161: LGTM: Added "change both" option for the channel configuration UI.This translation provides users with clear action text for modifying both platform settings.
212-215: LGTM: Added default channel configuration translations.New translations for default channel settings, including platform-specific options (Android/iOS) and synchronization features.
484-487: LGTM: Added channel selection dialog translations.These strings provide clear messaging for the default update channel selection workflow, including the special case when enabling synchronization.
tests/channel_self.test.ts (1)
245-253: Updated test to validate against new default channel structure.The test has been modified to check default channel configuration on the app level instead of the channel level, aligning with the database schema changes made for this PR.
🧰 Tools
🪛 ESLint
[error] 245-245: Should have line breaks between items, in node CallExpression
(antfu/consistent-chaining)
supabase/functions/_backend/utils/supabase.ts (2)
131-131: Enhanced return value to include channel ID even when no update is needed.This change improves the API by consistently providing the channel ID to callers, even when no changes were made.
135-146: Improved error handling and response for channel upsert operation.The code now properly awaits the upsert operation, extracts both data and error information, and provides better error handling with logging. The channel ID is consistently returned to callers when successful.
src/pages/app/p/[package].channel.[channel].vue (5)
27-27: Good addition of app_id reference in Channel interface.The Channel interface now properly references the apps table to access default channel information, which aligns with the change of moving default channel settings to the app table.
135-139: Good implementation of app data selection.The query now retrieves default channel information from the app record, correctly implementing the new schema design where default channels are stored at the app level.
217-217: Fixed app_id reference.The updated reference to
channel.value.app_id.app_idcorrectly adapts to the new data structure.
367-371: Good addition of redirect functionality.The new
redirectToAppSettingsfunction provides an intuitive way for users to navigate to the app settings page where they can manage default channels.
421-426: Good UI update for default channel display.The UI now displays whether a channel is set as default for either Android or iOS, with a convenient redirect button to app settings. This implementation aligns well with the new design where default channels are managed at the app level.
Consider extracting the conditional check
(channel.app_id.default_channel_android || channel.app_id.default_channel_ios) === idto a computed property for better readability:// Add this computed property in the script section const isDefaultChannel = computed(() => { if (!channel.value) return false; return (channel.value.app_id.default_channel_android || channel.value.app_id.default_channel_ios) === id.value; }); // Then in the template use: // {{ isDefaultChannel ? t('yes') : t('no') }}supabase/functions/_backend/public/channel/get.ts (1)
18-26: Good implementation for fetching app default channel data.The code now retrieves default channel information from the apps table, which aligns with the new schema design. Error handling is appropriate for cases where the app is not found.
tests/channel.test.ts (4)
28-39: Good test coverage for public field.This test case ensures that all channel responses include the public field, which is important for verifying the new functionality.
48-51: Good update to test expectations.The test now correctly checks for the presence and value of the public field in the response, ensuring the API returns the expected data structure.
74-75: Good addition of platform-specific properties.The test now includes iOS and Android properties when creating a channel, properly testing the new functionality.
82-98: Excellent test verification for default channels.This addition verifies that the default channels for both Android and iOS are correctly set in the database after creating a public channel. The verification is thorough and covers the expected behavior.
supabase/functions/_backend/public/channel/post.ts (1)
79-84: Improved error handling.The code now properly checks for errors from the updateOrCreateChannel function and handles them appropriately.
supabase/migrations/20250330054527_move_default_channel_to_app_table.sql (5)
1-11: Well-structured database schema changes for default channel migrationThe schema changes add appropriate foreign key columns and a synchronization flag to the apps table, which aligns well with the PR objective of moving default channel settings to the app level.
13-52: Comprehensive migration procedure with good error handlingThe migration procedure effectively populates the new columns by finding the most recent public channels for each platform and determining if they're synchronized. The approach of selecting the most recent public channels (using
ORDER BY updated_at DESC LIMIT 1) ensures the most relevant channels are selected as defaults.
54-57: Good cleanup of obsolete fieldsRemoving the
publiccolumn from the channels table is appropriate since this information has been migrated to the app level, eliminating redundancy in the database schema.
59-77: Appropriate foreign key constraints with deletion handlingThe addition of
ON DELETE SET NULLconstraints is an excellent practice, ensuring that if a channel is deleted, the app's reference simply becomes null rather than causing integrity issues or cascading deletions.
79-81: Proper constraint enforcementSetting the sync column to NOT NULL after populating it follows good database migration practices, ensuring data integrity while allowing for a smooth transition.
src/pages/app/p/[package].settings.vue (5)
32-32: Good addition of reactive variable for sync stateAdding a dedicated reactive variable to manage synchronization state improves the component's state management.
45-45: Expanded database query to include default channel informationThe query now selects the default channel information for both platforms, which is necessary for the new functionality.
60-60: Proper initialization of sync state from app dataThe sync state is correctly initialized from the app's data during the component's mounting phase.
727-735: Good toggle implementation for sync featureThe implementation of the synchronization toggle is clear and functional, providing a good user experience.
736-833: Platform-specific channel management UI when not syncedThe UI implementation for managing different channels for each platform when not synchronized is well-structured and provides a good user experience with clear labeling and visual grouping.
🧰 Tools
🪛 ESLint
[error] 755-755: Inconsistently quoted property '$reset' found.
(vue/quote-props)
[error] 784-784: Inconsistently quoted property '$reset' found.
(vue/quote-props)
messages/de.json (5)
123-124: New error messages for update channel operationsAdded appropriate German error messages for the new update channel functionality.
161-161: New action message for changing both platformsAdded the German translation for changing both platforms simultaneously.
212-215: Complete default channel label translationsAdded German translations for all the necessary default channel labels, maintaining consistency with the UI.
484-487: Comprehensive selection prompt translationsAdded German translations for the channel selection dialogs, including context-specific headers.
543-543: Success and unset messages for the update channel functionalityAdded German translations for the success messages and the "unset" action, completing the full set of translations needed for the feature.
Also applies to: 546-547
messages/pt-br.json (5)
123-124: New error messages for update channel operations in Brazilian PortugueseAdded appropriate Brazilian Portuguese error messages for the new update channel functionality.
161-161: Action message for managing both platformsAdded the Brazilian Portuguese translation for changing both platforms simultaneously.
212-215: Default channel label translationsAdded Brazilian Portuguese translations for all the necessary default channel labels.
484-487: Selection prompt translations with placeholder supportAdded Brazilian Portuguese translations for the channel selection dialogs, properly handling the $1 placeholder for dynamic text insertion.
543-543: Appropriate success and unset messagesAdded Brazilian Portuguese translations for the success messages and the "unset" action, completing the localization of the feature.
Also applies to: 546-547
messages/ru.json (3)
123-124: New Error Messages for Update Channel Changes
The added keys
•"cannot-change-update-channel": "Невозможно изменить канал обновления, пожалуйста, проверьте консоль браузера"
•"cannot-change-update-channel-sync": "Не удается изменить синхронизацию канала обновления, проверьте консоль браузера"
provide clear error reporting for update channel issues. Please verify these messages align with your overall error‐handling strategy.
161-161: "change-both" Message Clarity
The new key"change-both": "Измените оба"is concise and clear. Ensure this matches the UI prompt for changing both update channel parameters.
212-215: Default Channel Localization
The set of new keys:
•"default-android-channel": "Канал обновления по умолчанию для Android"
•"default-channel-sync": "Синхронизировать канал обновления по умолчанию для Android и iOS"
•"default-ios-channel": "Канал обновления по умолчанию для iOS"
•"default-update-channel": "Канал обновления по умолчанию"
are well added. Please verify that the terminology here (i.e. “обновления” vs. “загрузки”) is consistent across your codebase and other language files.messages/id.json (1)
212-215: Default Channel Localization
The default channel strings:
•"default-android-channel": "Saluran pembaruan default untuk Android"
•"default-channel-sync": "Menyinkronkan saluran pembaruan default untuk Android dan iOS"
•"default-ios-channel": "Saluran pembaruan default untuk iOS"
•"default-update-channel": "Saluran pembaruan default"
are properly localized. Confirm that these terms match your overall language strategy and are consistent with the translated UI.messages/fr.json (2)
161-161: "change-both" Localization
The key"change-both": "Modifier les deux"is concise and appears to accurately convey the action of changing both settings.
212-215: Default Channel Strings
The default channel keys:
•"default-android-channel": "Canal de mise à jour par défaut pour Android"
•"default-channel-sync": "Synchroniser le canal de mise à jour par défaut pour Android et iOS"
•"default-ios-channel": "Canal de mise à jour par défaut pour iOS"
•"default-update-channel": "Canal de mise à jour par défaut"
are appropriately localized. Please ensure that these are consistent with the other language files.messages/es.json (5)
123-124: Review of Update Channel Error Messages
The new error messages for keys"cannot-change-update-channel"and"cannot-change-update-channel-sync"are clear and consistent with the intended behavior. Please verify that the phrasing aligns with other error messages in the application.
161-161: Review of “change-both” Message
The message"change-both": "Cambiar ambos"is concise and clear. Ensure that its usage context in the UI is consistent with similar messages in other language files.
212-215: Review of Default Update Channel Messages
The translations for default channels:
"default-android-channel": "Canal de actualización por defecto para Android","default-channel-sync": "Sincronizar el canal de actualización por defecto para Android e iOS","default-ios-channel": "Canal de actualización por defecto para iOS", and"default-update-channel": "Canal de actualización predeterminado"
appear to be consistent and clear. Ensure that the terminology (“actualización”) is used uniformly throughout the UI.
484-487: Review of Default Update Channel Selection Prompts
The new messages for selecting the default update channel are descriptive. Please confirm that the placeholder variable$1is reliably interpolated at runtime and that the message tone matches other similar prompts.
543-543: Review of “unset” Key Translation
The new"unset": "Sin configurar"message is straightforward. Double-check its usage context to ensure it correctly conveys an unset (or not configured) status in the application.messages/pl.json (4)
123-124: Review of Update Channel Error Messages
The new Polish translations for the keys"cannot-change-update-channel"and"cannot-change-update-channel-sync"are clear. Please verify that they convey the same level of detail as in other language files.
161-161: Review of “change-both” Message
The message"change-both": "Zmień oba"is brief and clear. Confirm that it is used consistently with its counterparts in other locales.
212-215: Review of Default Update Channel Messages
The messages for default update channels are well translated. However, note that consistency in terminology (e.g. using “aktualizacji” for update) is important.
484-487: Review of Default Update Channel Selection Prompts
In the key"select-default-update-channel-header", the text"Wybierz domyślny kanał aktualizacji za 1 USD"hard-codes a price value. Please verify if this placeholder should instead be parameterized (e.g. using a variable like$1) to remain consistent with other locales.messages/vi.json (4)
123-124: Review of Update Channel Error Messages
The messages"cannot-change-update-channel"and"cannot-change-update-channel-sync"are clear and informative. Ensure that the phrasing meets the tone used in the Vietnamese version of similar messages.
161-161: Review of “change-both” Message
The Vietnamese translation"Thay đổi cả hai"for"change-both"is succinct and clear.
212-215: Review of Default Update Channel Messages
The messages for the default update channels for Android, iOS, and general update channels are consistent and clearly convey the information.
484-487: Review of Default Update Channel Selection Prompts
The messages here include a header with"với giá $ 1". Please verify if the pricing placeholder should be parameterized (e.g. as$1without extra spacing) to match the style in other language files.messages/ko.json (4)
123-124: New error message entries look good.These new entries provide appropriate Korean translations for error messages related to update channel changes, which aligns with the PR's goal of moving default channel configuration to the app table.
161-161: New action entry is appropriate.The "change-both" translation is concise and fits well with the UI interaction for updating both Android and iOS channel settings simultaneously.
212-215: Default channel setting translations are clear and consistent.These new translations properly describe the default channel configuration options for both platforms, including the synchronization feature mentioned in the PR objectives.
484-487: Channel selection UI translations are properly localized.The translations for the channel selection dialogs provide appropriate context for users, especially the detailed message explaining the sync behavior changes.
src/types/supabase.types.ts (6)
233-235: New app table fields support platform-specific default channels.Adding
default_channel_android,default_channel_ios, anddefault_channel_syncfields to the apps table correctly implements the PR's main objective of moving default channel configuration from individual channel records to the app level.
282-295: Appropriate foreign key relationships established.The foreign key relationships for
default_channel_androidanddefault_channel_iosare properly defined with ON DELETE SET NULL constraint (implied by the nullable fields), ensuring that if a channel is deleted, the application functionality remains intact as mentioned in the PR objectives.
396-396: Changedcreated_byfield to non-nullable in deploy_history.Making the
created_byfield required in the deploy_history table improves data integrity by ensuring all deployments are properly attributed to a user.Also applies to: 414-414, 426-426
575-598: Addeddeleted_appstable for tracking removed applications.The new
deleted_appstable allows for maintaining historical records of deleted applications, which is a good practice for auditing and potential recovery needs. This supports proper app lifecycle management.
1395-1407: Extendedget_app_metricsfunction to include deleted apps.The additional function overload for
get_app_metricsnow returns information about deleted apps, enhancing reporting capabilities and providing better visibility into application lifecycle statistics.
634-640: Added explicit foreign key relationships for deploy_history.The new foreign key relationships for
deploy_history.app_idanddeploy_history.created_byimprove the database schema by making relationships explicit, which enhances query performance and data integrity.Also applies to: 648-654
supabase/functions/_backend/utils/supabase.types.ts (8)
250-252: No issues found with the Insert definitions.
Having these properties optional is consistent with typical insert usage where fields can be omitted.
267-269: No issues found with the Update definitions.
The optional types align with partial updates in Supabase-type definitions.
396-396: Confirmcreated_bycolumn constraints in thechannelstable.
Currently, it’s defined as non-nullable in Row/Insert but optional in Update. Verify the database column is not nullable and that the code handles partial updates appropriately.Also applies to: 413-413, 430-430
604-604:created_byfordeploy_historynow required.
This adds clarity to each deploy’s creator. Ensure the database enforces a NOT NULL constraint if you intend never to allow nulls here.Also applies to: 615-615, 626-626
282-288:✅ Verification successful
Verify foreign key behavior for
default_channel_android.
Ensure the database migration sets an appropriate ON DELETE action (e.g.,SET NULLorRESTRICT), so references don’t break if the channel is removed.
🏁 Script executed:
#!/bin/bash # Check migrations for ON DELETE constraints on `apps_default_channel_android_fkey` rg -A 5 "apps_default_channel_android_fkey"Length of output: 1318
Foreign key behavior verified for
default_channel_androidThe migration file (
supabase/migrations/20250330054527_move_default_channel_to_app_table.sql) correctly re-adds theapps_default_channel_android_fkeyconstraint with the properON DELETE SET NULLaction. No further changes are needed.
289-295:✅ Verification successful
Verify foreign key behavior for
default_channel_ios.
Likewise, confirm that referencing channels for iOS default channel has the correct ON DELETE policy.
🏁 Script executed:
#!/bin/bash # Check migrations for ON DELETE constraints on `apps_default_channel_ios_fkey` rg -A 5 "apps_default_channel_ios_fkey"Length of output: 1267
Foreign Key ON DELETE Policy Verification for
default_channel_iosThe migration file (
supabase/migrations/20250330054527_move_default_channel_to_app_table.sql) correctly re-adds theapps_default_channel_ios_fkeywith anON DELETE SET NULLaction. Please confirm that this deletion behavior meets your intended constraint configuration for the iOS default channel reference in the channels table.
634-640:❓ Verification inconclusive
Review ON DELETE handling for
deploy_history_app_id_fkey.
Confirm your migration sets the desired behavior (e.g.,CASCADE,RESTRICT, orSET NULL).
🏁 Script executed:
#!/bin/bash # Verify the presence of ON DELETE clause for `deploy_history_app_id_fkey` rg -A 5 "deploy_history_app_id_fkey"Length of output: 36
Action Required: Verify ON DELETE behavior for "deploy_history_app_id_fkey" constraint.
Our current search in the code snippet (_backend/utils/supabase.types.ts) did not surface any explicit ON DELETE clause for this foreign key. Please manually verify that your migration explicitly defines the intended behavior (e.g., CASCADE, RESTRICT, or SET NULL) fordeploy_history_app_id_fkey. If this configuration is specified elsewhere (or intended to be managed by another process), kindly document that accordingly.
1395-1407:❓ Verification inconclusive
Ensure the new
get_app_metricssignature is properly implemented.
The additional return fielddeleted_appsmakes sense if your stored procedure or function logic calculates that. Please verify it actually computes and returns this value as intended.
🏁 Script executed:
#!/bin/bash # Check references to 'get_app_metrics' and confirm usage of new signature rg -A 5 "get_app_metrics"Length of output: 6535
Ensure the updated get_app_metrics signature is fully aligned across SQL and TypeScript.
Based on our repository search, there are multiple references to get_app_metrics as well as a migration (in supabase/migrations/20250317155158_stats_delete_app.sql) that appears to modify its behavior. Please verify that:
- The SQL implementation—in particular, the migration intended for deleted apps—actually computes and returns the new
deleted_appsfield.- The updated return type in
supabase/functions/_backend/utils/supabase.types.ts(which now includesdeleted_apps) matches what the SQL function returns.- All calls (for example, in
src/services/supabase.tsandsupabase/functions/_backend/public/statistics/index.ts) correctly use the updated signature.supabase/migrations/20240513055310_base.sql (5)
2073-2077: Channels Table Insertion Updated for New SchemaThis hunk inserts seed records into the channels table without the previously used public/is_public column. Instead, it now supplies:
- The channel name,
- An associated version number,
- Flags such as disable_auto_update_under_native and disable_auto_update (with a cast of the enum value
'major'),- And platform-specific booleans ios and android.
The provided values (for example, for the 'production' channel: ios = false and android = true) appear to match the new design. Confirm that these boolean settings align with the desired channel behavior in your UI.
2078-2078: Update Default Channels for Specific AppAn update statement explicitly sets the default channels for the app with app_id = 'com.demo.app':
- default_channel_ios is set to 3,
- default_channel_android is set to 1,
- default_channel_sync is explicitly set to false.
Make sure that the numeric IDs (3 and 1) indeed correspond to the correct channels (for example, matching the IDs generated in the seed data) and are in line with the new business rules.
2266-2287: Seeding Channels within reset_and_seed_app_data FunctionWithin the reset_and_seed_app_data function, the insertion into channels is performed using a VALUES clause defined as follows:
- Four tuples are provided:
•('production', '1.0.0', false, true, …)
•('beta', '1.361.0', false, true, …)
•('development', '1.359.0', true, false, …)
•('no_access', '1.361.0', false, false, …)This design clearly differentiates channels by name and sets the platform flags so that (later) the update uses the ‘production’ channel for Android and the ‘development’ channel for iOS.
Verify that these boolean values and version strings (e.g. the production channel uses version '1.0.0') match your intended release strategy.
2304-2309: Updating Seeded App’s Default ChannelsAt the end of the reset_and_seed_app_data function an update sets the default channels for the seeded app:
- default_channel_android is set using a subquery that selects the channel with name = 'production'.
- default_channel_ios is set using a subquery that selects the channel with name = 'development'.
This subquery‑based update helps ensure that the seeded application receives the correct defaults based on the channels you have just inserted. Make sure that the subqueries return unique values (or consider adding a
LIMIT 1if needed) and that these defaults are consistent with your overall product logic.
2053-2055:❓ Verification inconclusive
Default Channels Insertion into Apps Table
This insertion adds the three new columns—default_channel_ios, default_channel_android, and default_channel_sync—for applications. The values are set as follows:
- Both default_channel_ios and default_channel_android are inserted as NULL.
- default_channel_sync is set to true.
Please verify that these initial values reflect the intended behavior for new apps (e.g. that a NULL channel for iOS/Android is acceptable until later populated by the migration logic) and that the default sync flag (“true”) is indeed correct for your business logic.
Below is a shell script that extracts a few lines of context around the affected code to help verify that the inserted default values appear as expected:
Once the output of this script is confirmed, please verify that:
- The insertion sets default_channel_ios and default_channel_android to NULL, which is acceptable as a placeholder until later migration logic populates these values.
- The default_channel_sync flag is set to true as required by your business logic.
Verify Default Channel Values in SQL Migration
The migration file at
supabase/migrations/20240513055310_base.sql(lines 2053–2055) shows an INSERT statement that now includes three new columns:
•default_channel_iosis set to NULL
•default_channel_androidis set to NULL
•default_channel_syncis set to truePlease confirm that having NULL for the iOS/Android channels is acceptable until updated by later migration logic, and that the sync flag set to true aligns with your intended business rules.
🏁 Script executed:
#!/bin/bash # Print lines 2040 to 2060 from the migration file to check the insertion snippet context. sed -n '2040,2060p' supabase/migrations/20240513055310_base.sqlLength of output: 2602
Verify Default Channels Migration Values
The migration file at
supabase/migrations/20240513055310_base.sql(lines 2053–2055) now clearly includes three new columns in theappstable:
- default_channel_ios and default_channel_android are set to
NULLas placeholders.- default_channel_sync is set to
true.Please confirm that setting the iOS and Android channels to
NULLis acceptable until subsequent migration logic updates these values, and that the sync flag beingtruecorrectly reflects the intended business logic.messages/zh-cn.json (4)
123-124: New Update Channel Error Messages Added
The keys"cannot-change-update-channel"and"cannot-change-update-channel-sync"have been added to notify users if the update channel or its synchronization cannot be changed. Their messages correctly instruct the user to check the browser console for details.
161-161: Translation for "change-both" Key
The added"change-both"key with the value "同时更改" is clear and concise. Just ensure that this string aligns contextually with similar messages in other language files.
212-215: Default Update Channel Keys Introduced
The new keys for default update channels—"default-android-channel","default-channel-sync","default-ios-channel", and"default-update-channel"—are correctly added and follow the updated schema. Please verify that these translations are consistent with the intended default channel behavior.
543-543: Confirmation and Unset Messages
The keys"unset","updated-default-update-channel", and"updated-default-update-channel-sync"have been added to confirm successful operations and show unset status. Their messages appear appropriate; please double-check that the wording ("默认上传通道" vs. "默认更新通道同步") aligns with your backend behavior.Also applies to: 546-547
messages/tr.json (5)
123-124: New Update Channel Error Messages in Turkish
The keys"cannot-change-update-channel"and"cannot-change-update-channel-sync"are correctly added in Turkish. Their messages instruct users to check the browser console when an update channel change is not allowed.
161-161: "change-both" Translation Validation
The Turkish translation for"change-both"("Her ikisini de değiştirin") is clear. Ensure that it is consistent with related messaging in other localization files.
212-215: Default Update Channel Keys for Turkish
The new keys—"default-android-channel","default-channel-sync","default-ios-channel", and"default-update-channel"—are correctly localized. They reflect the intended default channel configuration for both platforms.
484-487: Default Update Channel Selection Prompts in Turkish
The keys for default channel selection ("select-default-update-channel","select-default-update-channel-header","select-default-update-channel-header-before-sync-change", and"select-default-update-channel-header-before-sync-change-message") are well translated. The placeholder usage appears to follow the convention (e.g., using "$1" if applicable). Double-check that dynamic parameters are handled correctly in your code.
543-543: Unset and Confirmation Messages in Turkish
The messages for"unset","updated-default-update-channel", and"updated-default-update-channel-sync"are properly localized. They communicate the required status feedback as intended.Also applies to: 546-547
messages/en.json (6)
10-11: Review of Newly Added "Override" Key
The"Override"key has been added here with the value "Override". This addition appears consistent with the intended functionality and matches similar keys in other language files.
119-124: Default Update Channel Error Handling
The updated keys"cannot-change-update-channel"and"cannot-change-update-channel-sync"are now part of the English localization. Their messages clearly instruct the user to check the browser console.
161-161: "change-both" Message Confirmation
The key"change-both"with the value "Change both" is straightforward and meets the requirements.
212-215: Default Update Channel Keys Added in English
The new default channel keys—"default-android-channel","default-channel-sync","default-ios-channel", and"default-update-channel"—are correctly introduced and use clear wording. Please verify that these entries align with the updated backend schema and UI expectations.
484-487: Dynamic Placeholder Usage in Default Channel Selection Header
The"select-default-update-channel-header"key is defined as "Select the default update channel for $1". This correctly uses a placeholder for dynamic insertion. The other keys for default channel selection are also clearly worded.
543-543: Review of Confirmation Messages for Update Operations
The keys"unset","updated-default-update-channel", and"updated-default-update-channel-sync"provide clear feedback to the user. One point to verify: the message for"updated-default-update-channel"mentions "default upload channel" rather than "default update channel"—please confirm that this is intentional based on your application’s behavior.Also applies to: 546-547
messages/it.json (2)
212-215: Addition of Default Channel Labels
The new default channel keys:
"default-android-channel": "Canale di aggiornamento predefinito per Android""default-channel-sync": "Sincronizzazione del canale di aggiornamento predefinito per Android e iOS""default-ios-channel": "Canale di aggiornamento predefinito per iOS""default-update-channel": "Canale di aggiornamento predefinito"These labels seem to support the restructured default channel configuration. Please ensure the wording (especially the use of “predefinito”) is consistent with other locales and UI terminology.
543-543: Confirmation of the "unset" Message
The updated key"unset": "Non impostato"is straightforward. Please confirm that this short message integrates well in the context where it is used, and that it is consistent with similar keys across other language files.
| "updated-default-update-channel": "¡Actualizado el canal de carga por defecto con éxito!", | ||
| "updated-default-update-channel-sync": "Actualizado correctamente la sincronización del canal de actualización predeterminada", |
There was a problem hiding this comment.
Critical: Inconsistency in Updated Default Channel Message
The key "updated-default-update-channel" currently has the text:
"¡Actualizado el canal de carga por defecto con éxito!"
It appears that the term "carga" (upload) is used instead of "actualización" (update). For consistency with the key and with other language versions, please update the message to reference the update channel rather than the upload channel.
| @@ -529,8 +540,11 @@ | |||
| "unlink-error": "Lỗi hủy liên kết, vui lòng kiểm tra bảng điều khiển", | |||
| "unlink-version": "Hủy liên kết phiên bản", | |||
| "unsafe": "Không an toàn", | |||
| "unset": "Unst", | |||
There was a problem hiding this comment.
Critical: “unset” Key Translation is Incorrect
The translation for "unset" is currently "Unst", which does not appear to be correct Vietnamese. Please update this key to a proper translation (for example, use a phrase like "chưa được thiết lập" or similar) to accurately convey the meaning of “unset.”
| "updated-default-update-channel": "Cập nhật kênh tải lên mặc định thành công!", | ||
| "updated-default-update-channel-sync": "Cập nhật thành công kênh cập nhật mặc định đồng bộ hóa", |
There was a problem hiding this comment.
Critical: Inconsistency in Updated Default Channel Message
The key "updated-default-update-channel" displays the message:
"Cập nhật kênh tải lên mặc định thành công!"
Here, "tải lên" (upload) is used instead of "cập nhật" (update). Please adjust the wording so that it correctly reflects that the default update channel was updated.
| { | ||
| foreignKeyName: "deploy_history_created_by_fkey" | ||
| columns: ["created_by"] | ||
| isOneToOne: false | ||
| referencedRelation: "users" | ||
| referencedColumns: ["id"] | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review ON DELETE handling for deploy_history_created_by_fkey.
Likewise, confirm that removing a user does not leave invalid references in deploy_history.
🏁 Script executed:
#!/bin/bash
# Verify the presence of ON DELETE clause for `deploy_history_created_by_fkey`
rg -A 5 "deploy_history_created_by_fkey"Length of output: 40
Let's gather additional context to verify if an ON DELETE handling clause is defined near the foreign key definition. Please run the following shell script to print lines 640–660 from the file:
🏁 Script executed:
#!/bin/bash
sed -n '640,660p' supabase/functions/_backend/utils/supabase.types.tsLength of output: 769
Action Required: Define ON DELETE Handling for deploy_history_created_by_fkey
Upon inspecting lines 640–660 in supabase/functions/_backend/utils/supabase.types.ts, it appears that the foreign key definition for deploy_history_created_by_fkey does not include an explicit ON DELETE clause. This omission means that when a user is removed, there’s no automated mechanism to update or remove dependent entries in deploy_history, which could leave invalid references.
- Current snippet:
{ foreignKeyName: "deploy_history_created_by_fkey" columns: ["created_by"] isOneToOne: false referencedRelation: "users" referencedColumns: ["id"] },
- Recommendation:
- Confirm whether this behavior is intentional.
- If deleting a user should trigger cascading changes (or a different controlled behavior, e.g., setting the reference to
NULL), update the foreign key definition to include the appropriate ON DELETE action (e.g.,ON DELETE CASCADEorON DELETE SET NULL). - Alternatively, ensure that your deletion logic in the application explicitly handles the removal or updating of related
deploy_historyentries.
| "updated-default-update-channel": "Aggiornato con successo il canale di caricamento predefinito!", | ||
| "updated-default-update-channel-sync": "Aggiornato con successo il canale di aggiornamento predefinito sync", |
There was a problem hiding this comment.
Inconsistency in Update Channel Success Message
The key "updated-default-update-channel" is intended to convey success related to the update channel; however, its message reads:
"Aggiornato con successo il canale di caricamento predefinito!"
This text refers to the "upload" channel rather than the "update" channel, which may lead to confusion. Please confirm if this is a typographical error and update the message accordingly—perhaps to something like "Aggiornato con successo il canale di aggiornamento predefinito!". Additionally, review the "updated-default-update-channel-sync" message to ensure consistent terminology.
…m/Cap-go/capgo into move_default_channel_to_app_table
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/cron_plan.test.ts (1)
4-23: Consider function declaration style for the retry utility.The retry function is well-implemented, but there's a style issue flagged by static analysis.
Consider declaring the function using the function keyword instead of an arrow function to align with the project's linting rules:
-// Helper function to retry tests with delay -const retryTest = (testFn: () => Promise<void>, attempts = 30, delayMs = 3000) => { - return async () => { +// Helper function to retry tests with delay +function retryTest(testFn: () => Promise<void>, attempts = 30, delayMs = 3000) { + return async () => {🧰 Tools
🪛 ESLint
[error] 5-5: Top-level functions should be declared with function keyword
(antfu/top-level-function)
[error] 12-12: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
src/pages/app/p/[package].settings.vue (2)
93-186: Complex synchronization logic with several edge cases.The
setUpdateChannelSyncfunction handles complex scenarios when enabling/disabling synchronization between iOS and Android default channels. While it works correctly, the function is quite long and complex.Consider breaking down this complex function into smaller, more focused functions for better maintainability:
async function setUpdateChannelSync(value: boolean) { if (value === true && appRef.value && needsChannelSelection()) { + await handleChannelSelectionBeforeSync(); - const channels = await supabase - .from('channels') - .select('*') - .eq('app_id', appId.value) - .in('id', [ - ...(appRef.value.default_channel_ios ? [(appRef.value.default_channel_ios as any).id] : []), - ...(appRef.value.default_channel_android ? [(appRef.value.default_channel_android as any).id] : []), - ]) - // ... rest of the dialog and selection logic } await updateSyncSetting(value); +} + +// Helper function to check if channel selection is needed +function needsChannelSelection() { + return appRef.value && ( + (appRef.value.default_channel_ios && appRef.value.default_channel_android && + (appRef.value.default_channel_ios as any)?.id !== (appRef.value.default_channel_android as any)?.id) || + (!appRef.value?.default_channel_ios && !!appRef.value?.default_channel_android) || + (!appRef.value?.default_channel_android && !!appRef.value?.default_channel_ios) + ); +} + +// Helper function to handle channel selection before enabling sync +async function handleChannelSelectionBeforeSync() { + const channels = await supabase + .from('channels') + .select('*') + .eq('app_id', appId.value) + .in('id', [ + ...(appRef.value.default_channel_ios ? [(appRef.value.default_channel_ios as any).id] : []), + ...(appRef.value.default_channel_android ? [(appRef.value.default_channel_android as any).id] : []), + ]) + // ... rest of the dialog and selection logic +} + +// Helper function to update the sync setting in the database +async function updateSyncSetting(value: boolean) { const { error } = await supabase.from('apps').update({ default_channel_sync: value }).eq('app_id', appId.value) if (error) { toast.error(t('cannot-change-update-channel-sync')) console.error(error) } if (appRef.value) { appRef.value.default_channel_sync = value forceBump.value += 1 } }
316-426: Well-structured function for managing default update channels.The
setDefaultUpdateChannelfunction provides a flexible way to set default channels for different platforms. It includes proper error handling and user feedback.Consider refactoring the repeated dialog setup code. Both
setDefaultUpdateChannelandsetDefaultChanneluse very similar dialog configurations.+ // Shared dialog configuration function + function createChannelSelectionDialog(header: string, buttons: any[]) { + displayStore.dialogOption = { + header, + message: `${t('select-default-update-channel')}`, + headerStyle: 'w-full text-center', + textStyle: 'w-full text-center', + preventAccidentalClose: true, + buttonVertical: true, + size: 'max-w-xl', + buttons: Array.prototype.concat(buttons, [{ text: t('button-cancel'), role: 'cancel' }]), + } + displayStore.showDialog = true + }supabase/migrations/20250330054527_move_default_channel_to_app_table.sql (4)
13-53: Procedure to Populate Default Channel Fields
The anonymous PL/pgSQL block iterates through all apps and selects the most recent channels that are both public and platform enabled.
- Correctness: The use of two separate SELECT queries (one for Android and one for iOS) and an update of each app record is functionally correct.
- Logic: The boolean expression for
is_syncedcorrectly checks that both channels exist and are the same—or that both are absent.- Performance Suggestion: For large datasets, a set-based UPDATE (using joins) may perform better than looping row-by-row; however, for one-time migrations this procedural approach is acceptable.
55-60: Commented-Out Column Drop Reminder
The commented section hints at a future removal of thepubliccolumn from the channels table. The annotated TODO date and remark explain why the column is still needed (for the CLI).
- Nitpick: Consider clarifying the migration plan further or linking to an issue so that future developers understand when and how to remove it.
87-123: Trigger Function: Update Channel Public Status from App
This trigger function ensures that if a default channel is set on an app, the corresponding channel record is marked as public.
- Logic Review:
- For each platform (iOS and Android), the function updates the channel to
public = TRUEif the new value is set.- The use of an
ELSIFbranch to update the channel topublic = FALSEwhen the value is removed is a simple approach.- Potential Improvement:
Consider the scenario where the default channel is updated (i.e. changed from one non-null value to another). With the current IF/ELSIF structure, only the new channel is marked as public while the old channel remains unchanged. If the intended behavior is to unmark the old channel when it is no longer default, you might split the conditions into separate IF blocks. For example:- IF NEW.default_channel_ios IS NOT NULL THEN - UPDATE public.channels - SET public = TRUE - WHERE id = NEW.default_channel_ios - AND app_id = NEW.app_id; - ELSIF OLD.default_channel_ios IS NOT NULL THEN - UPDATE public.channels - SET public = FALSE - WHERE id = OLD.default_channel_ios - AND app_id = NEW.app_id; - END IF; + IF OLD.default_channel_ios IS DISTINCT FROM NEW.default_channel_ios THEN + IF OLD.default_channel_ios IS NOT NULL THEN + UPDATE public.channels + SET public = FALSE + WHERE id = OLD.default_channel_ios + AND app_id = NEW.app_id; + END IF; + IF NEW.default_channel_ios IS NOT NULL THEN + UPDATE public.channels + SET public = TRUE + WHERE id = NEW.default_channel_ios + AND app_id = NEW.app_id; + END IF; + END IF;A similar adjustment can be applied for the Android default.
165-190: Procedure to Sync All Existing Channels
This final anonymous block iterates over all channels and updates those used as default in any app, ensuring that they are marked as public.
- Correctness: The loop and corresponding update statement are correct for ensuring data consistency.
- Performance Suggestion: For large datasets, a set-based approach (using a single UPDATE with a subquery) might be more efficient; however, for a migration script that runs once, this procedural approach is acceptable.
- Logging: The
RAISE NOTICEmessage provides useful feedback upon completion; consider including the count of updated rows if that detail would be useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/pages/app/p/[package].channel.[channel].vue(6 hunks)src/pages/app/p/[package].settings.vue(7 hunks)src/types/supabase.types.ts(5 hunks)supabase/functions/_backend/public/channel/get.ts(3 hunks)supabase/functions/_backend/utils/pg.ts(1 hunks)supabase/functions/_backend/utils/postgress_schema.ts(0 hunks)supabase/functions/_backend/utils/supabase.types.ts(5 hunks)supabase/migrations/20240513055310_base.sql(5 hunks)supabase/migrations/20250330054527_move_default_channel_to_app_table.sql(1 hunks)tests/cron_plan.test.ts(9 hunks)tests/updates.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- supabase/functions/_backend/utils/postgress_schema.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- supabase/functions/_backend/public/channel/get.ts
- supabase/functions/_backend/utils/pg.ts
- src/types/supabase.types.ts
- supabase/functions/_backend/utils/supabase.types.ts
- supabase/migrations/20240513055310_base.sql
🧰 Additional context used
🧬 Code Definitions (2)
tests/updates.test.ts (1)
tests/test-utils.ts (1)
getSupabaseClient(97-101)
tests/cron_plan.test.ts (1)
tests/test-utils.ts (3)
getSupabaseClient(97-101)BASE_URL(6-6)ORG_ID(9-9)
🪛 ESLint
tests/cron_plan.test.ts
[error] 5-5: Top-level functions should be declared with function keyword
(antfu/top-level-function)
[error] 12-12: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
🪛 GitHub Actions: Run tests
tests/cron_plan.test.ts
[error] 1-1: Test timed out in 60000ms. If this is a long-running test, pass a timeout value as the last argument or configure it globally with 'testTimeout'.
🔇 Additional comments (22)
tests/updates.test.ts (1)
278-278: Good schema change adaptation.The code correctly updates the test to reflect the new database schema where default channel settings moved from the
channelstable to theappstable. This is in line with the PR objective of centralizing default channel configurations at the application level.src/pages/app/p/[package].channel.[channel].vue (6)
9-9: LGTM! Added icon for the new redirect functionality.This import supports the new redirect functionality to app settings page.
27-27: Updated Channel interface to align with new data model.The interface now correctly references a row from the
appstable, following the schema changes where default channel information is stored at the app level.
135-139: Properly expanded database query to include default channel information.The query now correctly retrieves
id,default_channel_android, anddefault_channel_iosfrom theapp_idrelationship, enabling the channel page to display default channel status.
217-217: Updated property access to match new nested structure.The code correctly accesses
app_idthrough the new data structure following the schema changes.
367-371: Good addition of navigation function to app settings.This new function provides a clean way to navigate to the app settings page where users can manage default channels.
421-426: Well-implemented UI for displaying default channel status.The template has been updated to replace the old public status toggle with information about default channels and a redirect button to the app settings page, providing good user experience.
tests/cron_plan.test.ts (1)
520-578: Good restoration of test for deleted apps metrics.This previously commented-out test has been restored and updated to use the retry mechanism, improving test stability while ensuring that metrics for deleted apps are properly counted.
src/pages/app/p/[package].settings.vue (5)
10-11: LGTM! Added platform-specific icons.These imports support the new UI for platform-specific default channel settings.
31-31: Good addition of state for channel synchronization.The
defaultChannelSyncref will track whether iOS and Android channels should be kept in sync, supporting the new feature in the PR.
59-59: Properly initializes sync state from app settings.Good initialization of the synchronization state based on the app's configuration during component mounting.
738-746: Great UI implementation for channel synchronization toggle.The toggle for enabling/disabling channel synchronization is well implemented with proper styling and labels.
747-839: Well-designed UI for platform-specific channel settings.The implementation properly handles both synchronized and platform-specific views, providing a good user experience. When sync is enabled, a single channel selector is shown; when disabled, separate selectors for iOS and Android are displayed.
supabase/migrations/20250330054527_move_default_channel_to_app_table.sql (9)
1-4: Adddefault_channel_iosColumn
The SQL correctly adds the new column with an inline foreign key. Consider whether an explicit constraint name is needed for clarity in future migrations, although this implicit naming is acceptable.
5-8: Adddefault_channel_androidColumn
This segment mirrors the previous addition for iOS. The column is correctly defined with an inline foreign key reference.
9-12: Adddefault_channel_syncColumn
The boolean column is added with a default of false. Note that later the column is altered to be NOT NULL, which is consistent with ensuring a valid value in all rows.
63-69: Dropping Existing Foreign Key Constraints
The script drops existing FK constraints on the default channel columns. This is a necessary step to allow redefinition. The use ofIF EXISTSis appropriate to avoid errors if constraints are not present.
70-82: Re-Adding Foreign Key Constraints with ON DELETE SET NULL
The constraints are properly re-added with anON DELETE SET NULLaction, which meets the requirement to keep app records intact if a channel is deleted.
83-86: Enforcing NOT NULL ondefault_channel_sync
After populating the column with a default value, marking it NOT NULL guarantees consistency for future records.
124-131: Trigger on Apps Table
The script correctly drops any existing trigger before recreating it on thepublic.appstable. The trigger is set to fire AFTER INSERT or UPDATE for the default channel columns, ensuring that channel records update immediately after an app record changes.
132-156: Trigger Function: Manage Channel Public Status
This function checks if a channel is used as a default in any app (by verifying against both the iOS and Android default columns) and, if so, enforces that the channel’spublicflag remains TRUE.
- Logic: The use of
SELECT EXISTS (...) INTO is_defaultis effective to determine default usage.- Consideration: This function only adjusts the channel to public if it is default, but does not reset it if the channel is no longer default. This appears intentional based on the requirements.
157-164: Trigger on Channels Table
The trigger on thepublic.channelstable is correctly defined to fire BEFORE INSERT or UPDATE, ensuring that any changes to a channel’s default status via the apps table are enforced immediately.
| it('should return 400 when orgId is missing', { retry: 1 }, retryTest(async () => { | ||
| const response = await fetch(`${BASE_URL}/triggers/cron_plan`, { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the retry configuration consistency.
You're adding { retry: 1 } option while the retryTest function already has a default of 30 attempts. This might lead to confusion about how many retry attempts will actually occur.
🏁 Script executed:
#!/bin/bash
# Check if the retry option in test cases matches with the retry utility function
# Find all test cases using both retry option and retryTest
grep -A 3 "it.*{ retry: [0-9]* }.*retryTest" tests/cron_plan.test.ts
# Get default values from retryTest function
grep -A 1 "retryTest.*attempts =" tests/cron_plan.test.tsLength of output: 1880
Action Required: Align Retry Configurations in Tests
The tests in tests/cron_plan.test.ts use the { retry: 1 } option even though the retryTest helper defaults to 30 attempts. This creates ambiguity about which retry count is actually applied. Please review and adjust the configuration—either remove the redundant { retry: 1 } if you intend to rely on the 30-attempt default in retryTest or update the default in retryTest so both layers are consistent.
|
Can I test and merge? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/pages/app/p/[package].settings.vue (4)
93-94: Simplify complex conditional for better readability.The conditional check for sync-related edge cases is extremely long, making it difficult to understand and maintain.
Consider breaking down this complex condition into smaller, more readable parts:
- if (value === true && appRef.value && ((appRef.value.default_channel_ios && appRef.value.default_channel_android && (appRef.value.default_channel_ios as any)?.id !== (appRef.value.default_channel_android as any)?.id) || (!appRef.value?.default_channel_ios && !!appRef.value?.default_channel_android) || (!appRef.value?.default_channel_android && !!appRef.value?.default_channel_ios))) { + if (value === true && appRef.value) { + const hasIosChannel = !!appRef.value.default_channel_ios; + const hasAndroidChannel = !!appRef.value.default_channel_android; + const channelsDiffer = hasIosChannel && hasAndroidChannel && + (appRef.value.default_channel_ios as any)?.id !== (appRef.value.default_channel_android as any)?.id; + const onlyOneChannelSet = (!hasIosChannel && hasAndroidChannel) || (hasIosChannel && !hasAndroidChannel); + + if (channelsDiffer || onlyOneChannelSet) {
93-186: Refactor complex function for better maintainability.The
setUpdateChannelSyncfunction is quite complex with multiple responsibilities: checking conditions, fetching data, managing dialog state, handling user responses, and updating the database.Consider breaking this down into smaller, focused functions:
+ // Check if channels need synchronization resolution + function needsChannelResolution(): boolean { + if (!appRef.value) return false; + + const hasIosChannel = !!appRef.value.default_channel_ios; + const hasAndroidChannel = !!appRef.value.default_channel_android; + const channelsDiffer = hasIosChannel && hasAndroidChannel && + (appRef.value.default_channel_ios as any)?.id !== (appRef.value.default_channel_android as any)?.id; + const onlyOneChannelSet = (!hasIosChannel && hasAndroidChannel) || (hasIosChannel && !hasAndroidChannel); + + return channelsDiffer || onlyOneChannelSet; + } + + // Fetch relevant channels for the sync dialog + async function fetchChannelsForSync(): Promise<{data: any[], error: any} | null> { + if (!appRef.value) return null; + + const channelIds = [ + ...(appRef.value.default_channel_ios ? [(appRef.value.default_channel_ios as any).id] : []), + ...(appRef.value.default_channel_android ? [(appRef.value.default_channel_android as any).id] : []), + ]; + + return await supabase + .from('channels') + .select('*') + .eq('app_id', appId.value) + .in('id', channelIds); + } + + // Prompt user to select a unified channel + async function promptChannelSelection(channels: any[]): Promise<string | null> { + let buttonRole = null as string | null; + // Dialog setup code here... + + // Return selected channel ID or null + } + async function setUpdateChannelSync(value: boolean) { - if (value === true && appRef.value && ((appRef.value.default_channel_ios && appRef.value.default_channel_android && (appRef.value.default_channel_ios as any)?.id !== (appRef.value.default_channel_android as any)?.id) || (!appRef.value?.default_channel_ios && !!appRef.value?.default_channel_android) || (!appRef.value?.default_channel_android && !!appRef.value?.default_channel_ios))) { + if (value === true && needsChannelResolution()) { + const channelsResult = await fetchChannelsForSync(); + if (!channelsResult || channelsResult.error) { + toast.error(t('cannot-change-update-channel-sync')); + return; + } + + const selectedChannelId = await promptChannelSelection(channelsResult.data); + // Rest of the logic using selectedChannelId...This approach makes each piece of functionality easier to understand and test individually.
316-426: Consolidate duplicate code in the channel update function.The
setDefaultUpdateChannelfunction has repeated logic for handling different platform types, resulting in code duplication.Refactor to consolidate duplicate code:
async function setDefaultUpdateChannel(type: 'android' | 'ios' | 'both') { // Fetch channels code unchanged... // Consolidate the update logic + async function updateChannelInDatabase(channelId: number | null) { + let updatePayload = {}; + + if (type === 'both') { + updatePayload = { + default_channel_ios: channelId, + default_channel_android: channelId + }; + } else { + updatePayload = { + [`default_channel_${type}`]: channelId + }; + } + + return await supabase.from('apps') + .update(updatePayload) + .eq('app_id', appRef.value?.app_id ?? '') + .then(x => x.error); + } + + function updateLocalState(channelData: { id: number, name: string } | null) { + if (!appRef.value) return; + + if (type === 'both') { + appRef.value.default_channel_android = channelData as any; + appRef.value.default_channel_ios = channelData as any; + } else { + appRef.value[`default_channel_${type}`] = channelData as any; + } + forceBump.value += 1; + } // Buttons logic using the consolidated functions const buttons = channels.map((chann) => { return { text: chann.name, handler: async () => { - let appError: any - if (type !== 'both') { - appError = await supabase.from('apps') - .update({ [`default_channel_${type}`]: chann.id }) - .eq('app_id', appRef.value?.app_id ?? '') - .then(x => x.error) - } - else { - appError = await supabase.from('apps') - .update({ default_channel_ios: chann.id, default_channel_android: chann.id }) - .eq('app_id', appRef.value?.app_id ?? '') - .then(x => x.error) - } + const appError = await updateChannelInDatabase(chann.id); if (appError) { toast.error(t('cannot-change-update-channel')) console.error(error) return } - if (appRef.value && type !== 'both') { - appRef.value[`default_channel_${type}`] = { - id: chann.id, - name: chann.name, - } as any - forceBump.value += 1 - } - else if (appRef.value && type === 'both') { - appRef.value.default_channel_android = { - id: chann.id, - name: chann.name, - } as any - appRef.value.default_channel_ios = { - id: chann.id, - name: chann.name, - } as any - forceBump.value += 1 - } + updateLocalState({ + id: chann.id, + name: chann.name, + }); toast.success(t('updated-default-update-channel')) }, } }) // Similar refactoring for the unset handler
151-173: Error handling in channel synchronization could be improved.The channel selection and database update logic correctly handles errors, but the error handling could be more consistent and provide better user feedback.
Consider standardizing the error messages to provide more context to the user:
- if (!channel) { - console.error('Channel not found') - defaultChannelSync.value = !value - return - } + if (!channel) { + console.error('Channel not found in selection results') + toast.error(t('cannot-change-update-channel-sync')) + defaultChannelSync.value = !value + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
messages/de.json(5 hunks)messages/en.json(5 hunks)messages/es.json(5 hunks)messages/fr.json(5 hunks)messages/id.json(5 hunks)messages/it.json(5 hunks)messages/ja.json(5 hunks)messages/ko.json(5 hunks)messages/pl.json(5 hunks)messages/pt-br.json(5 hunks)messages/ru.json(5 hunks)messages/tr.json(5 hunks)messages/vi.json(5 hunks)messages/zh-cn.json(5 hunks)src/components/tables/ChannelTable.vue(0 hunks)src/pages/app/p/[package].channel.[channel].vue(6 hunks)src/pages/app/p/[package].settings.vue(7 hunks)supabase/functions/_backend/utils/pg.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/tables/ChannelTable.vue
✅ Files skipped from review due to trivial changes (5)
- messages/fr.json
- messages/ja.json
- messages/zh-cn.json
- messages/it.json
- messages/vi.json
🚧 Files skipped from review as they are similar to previous changes (11)
- src/pages/app/p/[package].channel.[channel].vue
- supabase/functions/_backend/utils/pg.ts
- messages/tr.json
- messages/en.json
- messages/de.json
- messages/es.json
- messages/pl.json
- messages/ru.json
- messages/pt-br.json
- messages/ko.json
- messages/id.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (5)
src/pages/app/p/[package].settings.vue (5)
107-107: Good! Variable declaration uses modern syntax.You've correctly used
letfor variable declaration, which is the preferred approach in modern JavaScript.
747-808: UI implementation for platform-specific channel settings is well-structured.The conditional rendering for iOS and Android channels when synchronization is disabled is well organized. Each platform has its own FormKit component with appropriate icons and edit buttons.
809-839: Good implementation of synchronized channel UI.The UI for the synchronized default update channel includes the appropriate icon and edit button. The toggle between synchronized and platform-specific views is well implemented.
738-746: Toggle implementation for channel synchronization is well done.The checkbox implementation for toggling between synchronized and platform-specific default channels is clean and user-friendly.
58-59: Good initialization of synchronization state.The initialization of
defaultChannelSyncfrom the app record is well implemented with an appropriate fallback value.
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/cron_plan.test.ts (1)
132-132: Action Required: Align Retry Configurations in TestsThe tests in
tests/cron_plan.test.tsuse the{ retry: 1 }option even though theretryTesthelper defaults to 30 attempts. This creates ambiguity about which retry count is actually applied. Please review and adjust the configuration—either remove the redundant{ retry: 1 }if you intend to rely on the 30-attempt default inretryTestor update the default inretryTestso both layers are consistent.
🧹 Nitpick comments (5)
tests/channel.test.ts (3)
25-25: Avoid committingconsole.logcalls to the test suiteLeft-over
console.logstatements add noise to CI logs and can easily mask more relevant diagnostic output. They should be removed (or replaced with an assertion if you really need to inspect something).- console.log(data)
30-41: Strengthen the public-field assertionYou only check that
publicis defined.
Given the new semantics (publicis always a boolean derived by the backend), it is safer to assert its type as well:- for (const o of data as any[]) { - expect(o.public).toBeDefined() - } + for (const o of data as any[]) { + expect(typeof o.public).toBe('boolean') + }This prevents a silent regression where the field accidentally becomes, for example,
nullor a string.
70-78:publicis obsolete in the POST payloadAccording to the new API contract, the
publicbody parameter is ignored and default-channel logic happens through theandroid/iosflags. Keeping the stale field:
- Makes the test harder to read (future maintainers will wonder why it is there).
- Risks re-introducing server-side schema drift if somebody decides to “honour” it again.
- public: true, android: true, ios: true,(If the backend still requires the key due to backward-compat code that was not yet cleaned up, add a TODO explaining why).
supabase/migrations/20240513055310_base.sql (2)
2269-2290: Refactor channel seeding inreset_and_seed_app_data.
TheINSERTintochannelsuses a hardcodedVALUESlist forproduction,beta,development, andno_access. Consider extracting this list into a structured constant or JSON/array parameter to reduce duplication and improve maintainability if channel definitions evolve.
2307-2312: Use CTE results to set default channels directly.
Currently, the function updatesappswithSELECT id FROM channels WHERE name = .... Sinceinserted_channelsCTE already returns channel IDs, you could reuse its output to setdefault_channel_androidanddefault_channel_iosin a single CTE operation, avoiding separate subqueries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (26)
messages/de.json(5 hunks)messages/en.json(5 hunks)messages/es.json(5 hunks)messages/fr.json(5 hunks)messages/id.json(5 hunks)messages/it.json(5 hunks)messages/ja.json(5 hunks)messages/ko.json(5 hunks)messages/pl.json(5 hunks)messages/pt-br.json(5 hunks)messages/ru.json(5 hunks)messages/tr.json(5 hunks)messages/vi.json(5 hunks)messages/zh-cn.json(5 hunks)src/components/dashboard/AppSetting.vue(6 hunks)src/components/tables/ChannelTable.vue(0 hunks)src/pages/app/p/[package].channel.[channel].vue(6 hunks)src/types/supabase.types.ts(4 hunks)supabase/functions/_backend/utils/pg.ts(1 hunks)supabase/functions/_backend/utils/postgress_schema.ts(0 hunks)supabase/functions/_backend/utils/supabase.ts(1 hunks)supabase/functions/_backend/utils/supabase.types.ts(4 hunks)supabase/migrations/20240513055310_base.sql(5 hunks)tests/channel.test.ts(3 hunks)tests/channel_self.test.ts(1 hunks)tests/cron_plan.test.ts(9 hunks)
💤 Files with no reviewable changes (2)
- supabase/functions/_backend/utils/postgress_schema.ts
- src/components/tables/ChannelTable.vue
🚧 Files skipped from review as they are similar to previous changes (20)
- supabase/functions/_backend/utils/pg.ts
- supabase/functions/_backend/utils/supabase.ts
- tests/channel_self.test.ts
- messages/tr.json
- src/types/supabase.types.ts
- messages/pl.json
- supabase/functions/_backend/utils/supabase.types.ts
- src/pages/app/p/[package].channel.[channel].vue
- messages/zh-cn.json
- messages/vi.json
- messages/id.json
- messages/de.json
- messages/en.json
- messages/ja.json
- messages/fr.json
- messages/pt-br.json
- messages/es.json
- messages/ko.json
- messages/ru.json
- messages/it.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/channel.test.ts (1)
tests/test-utils.ts (2)
BASE_URL(6-6)getSupabaseClient(98-102)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (5)
tests/cron_plan.test.ts (2)
5-25: Good implementation of retry mechanism for flaky testsThe
retryTesthelper function is well-implemented with proper error handling and configurable retry attempts. This will help improve test reliability, especially for tests involving network requests and database operations.
523-581: Good restoration of deleted app metrics testThe uncommented test case for verifying that metrics are correctly counted for deleted apps is an important addition to ensure deleted apps are still considered in organizational metrics. This helps validate that the plan limits are enforced correctly even after app deletion.
supabase/migrations/20240513055310_base.sql (3)
2056-2058: Verify seed data for default channels on existing apps.
The seed inserts forcom.demoadmin.appandcom.demo.appsetdefault_channel_iosanddefault_channel_androidtoNULLwithdefault_channel_sync = true. Please confirm that downstream logic correctly handlesNULLdefaults when synchronization is enabled, falling back to the appropriate channel IDs.
2076-2079: Confirm channel ios/android flags in seeded channels.
The seeded channels forcom.demo.appuse platform flags (ios,android) to distinguish targets. Ensure that these boolean flags map to the intended platforms and that UI/API logic filters channels by these properties correctly.
2081-2081: Seedcom.demo.appdefault channel update looks correct.
TheUPDATEsetsdefault_channel_ios = 3,default_channel_android = 1, anddefault_channel_sync = false, matching the seeded channels above. This aligns with the new schema.
| async function setUpdateChannelSync(value: boolean) { | ||
| if (value === true && appRef.value && ((appRef.value.default_channel_ios && appRef.value.default_channel_android && (appRef.value.default_channel_ios as any)?.id !== (appRef.value.default_channel_android as any)?.id) || (!appRef.value?.default_channel_ios && !!appRef.value?.default_channel_android) || (!appRef.value?.default_channel_android && !!appRef.value?.default_channel_ios))) { | ||
| const channels = await supabase | ||
| .from('channels') | ||
| .select('*') | ||
| .eq('app_id', props.appId) | ||
| .in('id', [ | ||
| ...(appRef.value.default_channel_ios ? [(appRef.value.default_channel_ios as any).id] : []), | ||
| ...(appRef.value.default_channel_android ? [(appRef.value.default_channel_android as any).id] : []), | ||
| ]) | ||
| if (channels.error) { | ||
| toast.error(t('cannot-change-update-channel-sync')) | ||
| return | ||
| } | ||
| let buttonRole = null as string | null | ||
| displayStore.dialogOption = { | ||
| header: t('select-default-update-channel-header-before-sync-change'), | ||
| message: t('select-default-update-channel-header-before-sync-change-message'), | ||
| buttons: [ | ||
| ...channels.data.map(channel => ({ | ||
| text: channel.name, | ||
| handler: () => { | ||
| buttonRole = channel.id.toString() | ||
| }, | ||
| })), | ||
| ...(!appRef.value.default_channel_ios || !appRef.value.default_channel_android | ||
| ? [{ | ||
| text: t('undefined'), | ||
| role: 'danger', | ||
| handler: () => { | ||
| buttonRole = 'empty' | ||
| }, | ||
| }] | ||
| : []), | ||
| { | ||
| text: t('button-cancel'), | ||
| role: 'cancel', | ||
| }, | ||
| ], | ||
| preventAccidentalClose: true, | ||
| buttonVertical: true, | ||
| headerStyle: 'w-full text-center', | ||
| textStyle: 'min-w-full! text-center', | ||
| size: 'max-w-3xl', | ||
| buttonCenter: true, | ||
| } | ||
| displayStore.showDialog = true | ||
| const res = await displayStore.onDialogDismiss() | ||
| if (res) { | ||
| console.log('Dialog dismissed') | ||
| defaultChannelSync.value = !value | ||
| return | ||
| } | ||
| if (!buttonRole) { | ||
| console.error('No button role') | ||
| defaultChannelSync.value = !value | ||
| return | ||
| } | ||
| const channel = buttonRole === 'empty' ? { id: null, name: null } : channels.data.find(chann => chann.id.toString() === buttonRole) | ||
| if (!channel) { | ||
| console.error('Channel not found') | ||
| defaultChannelSync.value = !value | ||
| return | ||
| } | ||
| const { error: appError } = await supabase.from('apps') | ||
| .update({ default_channel_ios: channel.id, default_channel_android: channel.id }) | ||
| .eq('app_id', props.appId) | ||
| if (appError) { | ||
| toast.error(t('cannot-change-update-channel-sync')) | ||
| console.error(appError) | ||
| return | ||
| } | ||
| defaultChannelSync.value = value | ||
| if (appRef.value) { | ||
| if (channel) { | ||
| appRef.value.default_channel_ios = channel as any | ||
| appRef.value.default_channel_android = channel as any | ||
| } | ||
| else { | ||
| appRef.value.default_channel_ios = null | ||
| appRef.value.default_channel_android = null | ||
| } | ||
| } | ||
| forceBump.value += 1 | ||
| toast.success(t('updated-default-update-channel-sync')) | ||
| } | ||
| const { error } = await supabase.from('apps') | ||
| .update({ default_channel_sync: value }) | ||
| .eq('app_id', props.appId) | ||
| if (error) { | ||
| toast.error(t('cannot-change-update-channel-sync')) | ||
| console.error(error) | ||
| } | ||
| if (appRef.value) { | ||
| appRef.value.default_channel_sync = value | ||
| forceBump.value += 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Cancelling the sync-change dialog still updates the DB
If the user cancels or closes the confirm dialog, you revert the local checkbox (defaultChannelSync.value = !value) but execution continues and the code below (lines 170-179) still issues
await supabase.from('apps')
.update({ default_channel_sync: value })This permanently flips the flag in the database – the exact opposite of what the user asked for.
Early-return on cancellation to avoid the write:
- if (res) {
- console.log('Dialog dismissed')
- defaultChannelSync.value = !value
- return
- }
+ if (res) { // user clicked “cancel” (or backdrop)
+ defaultChannelSync.value = !value
+ return // ← stop here, DO NOT update Supabase
+ }You’ll also want the same early return in the two other places where you revert defaultChannelSync.value (!buttonRole and channel not found).
🤖 Prompt for AI Agents
In src/components/dashboard/AppSetting.vue between lines 84 and 181, when the
user cancels or closes the sync-change dialog, the function reverts the local
checkbox state but continues to update the database, which is incorrect. To fix
this, add early return statements immediately after reverting
defaultChannelSync.value in the cancellation check, the no buttonRole check, and
the channel not found check to prevent further execution and avoid updating the
database when the user cancels or an error occurs.
There was a problem hiding this comment.
Well, no, the return is here already ??
There was a problem hiding this comment.
@WcaleNieWolny You're absolutely right! I apologize for the confusion in my review.
Looking at the code structure again, I can see that there are return statements handling the dialog cancellation properly:
if (res) {
console.log('Dialog dismissed')
defaultChannelSync.value = !value
return // ← This DOES prevent further execution
}The database update code at the bottom (await supabase.from('apps').update({ default_channel_sync: value })) is intentionally outside the conflict-resolution if block, so it correctly runs for:
- Enabling sync when there are no conflicts (skips dialog)
- Disabling sync (skips dialog)
- Enabling sync after user resolves conflicts via dialog
But it properly skips when the user cancels the dialog due to the return statements you mentioned.
My initial analysis was incorrect - the code flow is actually working as intended. Thank you for the correction!
🐰✨
/| /|
( ✌️ ✌️ )
)_) (_/(
I learned something!
✏️ Learnings added
Learnt from: WcaleNieWolny
PR: Cap-go/capgo#1107
File: src/components/dashboard/AppSetting.vue:85-182
Timestamp: 2025-05-29T18:21:17.686Z
Learning: In setUpdateChannelSync function in src/components/dashboard/AppSetting.vue, the database update code at the end is intentionally outside the conflict-resolution if block and should run for cases where sync is being toggled without conflicts or after conflicts are resolved. The return statements inside the dialog handling correctly prevent database updates when the user cancels.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…/capgo into move_default_channel_to_app_table
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/dashboard/AppSetting.vue (2)
133-137:⚠️ Potential issueAdd early returns to prevent database updates after cancellation.
When the user cancels the dialog or an error occurs, the function reverts the UI state but continues to update the database, causing inconsistency.
Add early returns after reverting the checkbox state:
if (res) { console.log('Dialog dismissed') defaultChannelSync.value = !value + return - return }if (!buttonRole) { console.error('No button role') defaultChannelSync.value = !value + return - return }if (!channel) { console.error('Channel not found') defaultChannelSync.value = !value + return - return }Also applies to: 138-142, 144-148
747-747:⚠️ Potential issueUse the current value from v-model instead of stale appRef value.
The
@changehandler uses the stale previous value fromappRef, which causes toggle issues after the first change.Since you're already using
v-model="defaultChannelSync", pass the current value:- @change="setUpdateChannelSync(!appRef?.default_channel_sync)" + @change="setUpdateChannelSync(defaultChannelSync)"
🧹 Nitpick comments (2)
supabase/functions/_backend/utils/pg.ts (1)
259-269: Fix indentation to match project style.The ESLint errors indicate incorrect indentation. The conditions should be indented with 10 spaces and the closing parentheses with 8 spaces.
Apply this diff to fix the indentation:
.where(!defaultChannel ? and( - sql`(SELECT (apps.default_channel_android = ${channelAlias.id} OR apps.default_channel_ios = ${channelAlias.id}) FROM ${schema.apps} WHERE app_id = ${app_id}) = true`, - eq(channelAlias.app_id, app_id), - eq(platform === 'android' ? channelAlias.android : channelAlias.ios, true), - ) - : and( - eq(channelAlias.app_id, app_id), - eq(channelAlias.name, defaultChannel), - ), + sql`(SELECT (apps.default_channel_android = ${channelAlias.id} OR apps.default_channel_ios = ${channelAlias.id}) FROM ${schema.apps} WHERE app_id = ${app_id}) = true`, + eq(channelAlias.app_id, app_id), + eq(platform === 'android' ? channelAlias.android : channelAlias.ios, true), + ) + : and( + eq(channelAlias.app_id, app_id), + eq(channelAlias.name, defaultChannel), + ), )And similarly for the V2 function:
.where(!defaultChannel ? and( - sql`(SELECT (apps.default_channel_android = ${channelAlias.id} OR apps.default_channel_ios = ${channelAlias.id}) FROM ${schemaV2.apps} WHERE app_id = ${app_id}) = true`, - eq(channelAlias.app_id, app_id), - eq(platform === 'android' ? channelAlias.android : channelAlias.ios, true), - ) - : and( - eq(channelAlias.app_id, app_id), - eq(channelAlias.name, defaultChannel), - ), + sql`(SELECT (apps.default_channel_android = ${channelAlias.id} OR apps.default_channel_ios = ${channelAlias.id}) FROM ${schemaV2.apps} WHERE app_id = ${app_id}) = true`, + eq(channelAlias.app_id, app_id), + eq(platform === 'android' ? channelAlias.android : channelAlias.ios, true), + ) + : and( + eq(channelAlias.app_id, app_id), + eq(channelAlias.name, defaultChannel), + ), )Also applies to: 385-395
🧰 Tools
🪛 ESLint
[error] 261-261: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 262-262: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 263-263: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 264-264: Expected indentation of 8 spaces but found 6.
(style/indent)
[error] 266-266: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 267-267: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 268-268: Expected indentation of 8 spaces but found 6.
(style/indent)
supabase/functions/_backend/public/channel/post.ts (1)
104-104: Fix brace style to follow project conventions.The closing braces should be on separate lines from the subsequent blocks.
Apply this diff to fix the brace style:
} - } else { + } + else {And:
} return c.json(BRES) - } catch (e) { + } + catch (e) {Also applies to: 130-130
🧰 Tools
🪛 ESLint
[error] 104-104: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/dashboard/AppSetting.vue(7 hunks)supabase/functions/_backend/public/channel/post.ts(1 hunks)supabase/functions/_backend/utils/pg.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
supabase/functions/_backend/public/channel/post.ts (2)
supabase/functions/_backend/utils/supabase.ts (2)
updateOrCreateChannel(110-147)supabaseAdmin(59-68)supabase/functions/_backend/utils/hono.ts (1)
BRES(208-208)
🪛 ESLint
supabase/functions/_backend/public/channel/post.ts
[error] 104-104: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 130-130: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
supabase/functions/_backend/utils/pg.ts
[error] 261-261: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 262-262: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 263-263: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 264-264: Expected indentation of 8 spaces but found 6.
(style/indent)
[error] 266-266: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 267-267: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 268-268: Expected indentation of 8 spaces but found 6.
(style/indent)
[error] 387-387: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 388-388: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 389-389: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 390-390: Expected indentation of 8 spaces but found 6.
(style/indent)
[error] 392-392: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 393-393: Expected indentation of 10 spaces but found 8.
(style/indent)
[error] 394-394: Expected indentation of 8 spaces but found 6.
(style/indent)
🔇 Additional comments (2)
supabase/functions/_backend/public/channel/post.ts (1)
79-129: Well-implemented default channel management logic!The code correctly handles both setting and unsetting default channels based on the public flag, addressing the previous review comments. The implementation properly checks platform support before updating defaults.
🧰 Tools
🪛 ESLint
[error] 104-104: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
src/components/dashboard/AppSetting.vue (1)
312-422: Excellent implementation of default channel management UI!The new functionality provides a clean interface for managing default channels with proper synchronization controls. The conditional rendering based on sync state and the ability to set/unset channels per platform or synchronously is well implemented.
Also applies to: 742-843
| @@ -2,6 +2,28 @@ import { randomUUID } from 'node:crypto' | |||
| import { afterAll, beforeEach, describe, expect, it } from 'vitest' | |||
| import { BASE_URL, getBaseData, getSupabaseClient, postUpdate, PRODUCT_ID, resetAndSeedAppDataStats, resetAppData, resetAppDataStats, TEST_EMAIL, USER_ID } from './test-utils.ts' | |||
|
|
|||
| // Helper function to retry tests with delay | |||
| function retryTest(testFn: () => Promise<void>, attempts = 30, delayMs = 3000) { | |||
There was a problem hiding this comment.
It made the tests work, like, actually retry. It was a brute force method to get the tests to work, I think. I don't fully remember
…/capgo into move_default_channel_to_app_table
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/pages/app/p/[package].channel.[channel].vue(6 hunks)src/types/supabase.types.ts(4 hunks)supabase/functions/_backend/utils/pg.ts(2 hunks)supabase/functions/_backend/utils/supabase.ts(1 hunks)supabase/functions/_backend/utils/supabase.types.ts(4 hunks)supabase/seed.sql(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- supabase/functions/_backend/utils/supabase.ts
- src/types/supabase.types.ts
- supabase/functions/_backend/utils/pg.ts
- src/pages/app/p/[package].channel.[channel].vue
- supabase/functions/_backend/utils/supabase.types.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (5)
supabase/seed.sql (5)
99-101: Initialize default channel fields for seeded apps
TheINSERT INTO public.appsnow includes the newdefault_channel_ios,default_channel_android, anddefault_channel_synccolumns. Setting both channel IDs toNULLwithdefault_channel_sync = trueis sensible for demo apps that start in synced mode without a preselected channel.
119-123: Seed channel entries without the oldpublicflag
TheINSERT INTO public.channelsblock correctly omits the removedis_publiccolumn, uses't'fordisable_auto_update_under_nativeand'major'::disable_updatefordisable_auto_update, and sets theios/androidbooleans as intended. This aligns with the new schema.
124-124: Set per-app default channels forcom.demo.app
TheUPDATE public.appsstatement assigns channel ID 3 for iOS, channel ID 1 for Android, and togglesdefault_channel_synctofalseas expected for this demo app.
317-317: Align reset function’s channel insert with schema changes
Inreset_and_seed_app_data, theINSERT INTO public.channelsnow matches the updated column list (withoutpublic), ensuring the reset flow won’t break after the migration.
334-338: Define channel tuples for seeded app data
The CTE’sVALUESclause correctly enumeratesproduction,beta,development, andno_accesschannels with the rightversion_name,ios/androidflags, andcreated_byUUIDs.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
feat: Move default channel to app table
Summary
Overview
This PR implements the first part of moving default channel configurations from individual channel records to the application level. This architectural change allows for more centralized management of default channel settings and improves the update delivery workflow.
Changes
Database Schema:
Added new columns to the apps table:
default_channel_ios: Foreign key reference to channels table
default_channel_android: Foreign key reference to channels table
default_channel_sync: Boolean flag to control if iOS and Android channels are synchronized
Removed public column from the channels table
Added migration script to populate these new fields for existing apps
Backend Changes:
Updated API endpoints in channel/get.ts and channel/post.ts to handle the new structure
Modified database utility functions to support the new schema
Updated Supabase type definitions to reflect the database changes
UI Enhancements:
Redesigned app settings page with dedicated section for managing default channels
Added UI for toggling channel sync between platforms
Implemented separate controls for iOS and Android default channels when not synced
Added dialogs for channel selection and confirmation
Localization:
Added new translation keys for default channel settings
Updated existing translations in multiple languages
Technical Details
ON DELETE SET NULL constraints added to foreign key references, ensuring apps can still function if a channel is deleted
Migration automatically determines if existing apps have synced channels and sets the flag accordingly
Added channel sync toggle in the UI with confirmation dialog when switching from separate to synced mode
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores