feat: remove usage of 'public' field from channel table #400
Conversation
WalkthroughThe changes update the application's channel management and database schema. In the API and bundle modules, functions now perform concurrent queries to retrieve channel data and the default channel identifiers, with enhanced error handling and updated logic to designate channels as public. The channel setting routine now includes additional logic and user prompts to update default channels based on visibility. The database type definitions have been modified to include new properties, functions, and a new table, while minor syntax standardization was applied in utility functions. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant G as getActiveChannels
participant DB1 as Channels Table
participant DB2 as Apps Table
C->>G: Call getActiveChannels(appid)
par Concurrent Queries
G->>DB1: Query channels data
G->>DB2: Query default channel IDs
end
DB1-->>G: Return channels data
DB2-->>G: Return default channel IDs
G->>G: Check for errors in both responses
G->>G: Set 'public' flag based on default IDs
G-->>C: Return enhanced channels data
sequenceDiagram
participant U as Caller (setChannel)
participant S as setChannel Function
participant DB as Supabase Database
participant P as User Prompt
U->>S: Call setChannel(payload)
S->>S: Determine publicChannel flag
S->>DB: Retrieve current default channels (iOS/Android)
alt Channel set to private and is default
S->>DB: Clear default channel(s) and update sync flag
else Channel already public for both platforms
S->>S: Log status message
else Channel set to public or user prompt needed
S->>P: Prompt user for platform selection
P-->>S: User selects platform(s)
S->>DB: Update default channels based on selection
end
S-->>U: Return updated channel status
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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 (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/channel/set.ts (1)
158-249: Conditional default channel updating flow
This block handles setting the channel as “public” across iOS, Android, or both, with user prompts to decide. The stepwise logic is clear and provides robust error handling. Extracting platform update code into helper functions could enhance readability, but overall it’s well-structured.src/api/channels.ts (1)
165-169: Includedefault_channel_syncif relevant
Currently, onlydefault_channel_iosanddefault_channel_androidare selected. If your workflow also needsdefault_channel_sync, you might consider adding it to this query.supabase.from('apps') - .select('default_channel_ios, default_channel_android') + .select('default_channel_ios, default_channel_android, default_channel_sync') .eq('app_id', appid) .single()src/types/supabase.types.ts (2)
578-601: Newdeleted_appstable
Adding a dedicated table to store deleted apps is a good practice. If app references are needed, consider adding indexes or relationships.
2157-2168:reset_and_seed_app_dataandreset_and_seed_app_stats_data
These reset/seed functions are often destructive. Ensure you protect them from accidental usage in production or logs are thorough.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/channels.ts(2 hunks)src/bundle/upload.ts(2 hunks)src/channel/set.ts(4 hunks)src/types/supabase.types.ts(13 hunks)src/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/bundle/upload.ts (1)
src/utils.ts (1)
formatError(45-45)
src/channel/set.ts (1)
src/utils.ts (2)
updateOrCreateChannel(966-994)formatError(45-45)
🪛 ESLint
src/api/channels.ts
[error] 173-173: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
🪛 GitHub Actions: Run tests
src/api/channels.ts
[error] 173-173: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
🪛 GitHub Actions: autofix.ci
src/api/channels.ts
[error] 173-173: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
🔇 Additional comments (24)
src/utils.ts (3)
470-480: Retrieve app default channels carefully
This block correctly fetches thedefault_channel_iosanddefault_channel_androidcolumns from theappstable, then handles any error by logging it and exiting. The approach is fine. Just be sure to confirm, in upstream code, thatappDataindeed contains valid fields (e.g., if the app record exists but lacks these columns).
580-580: Use ofyield*for generator chaining
Replacingyield * getFiles(res)withyield* getFiles(res)aligns with modern JavaScript syntax and keeps generator delegation consistent.
786-786: Consistentyield*usage
Similarly updatingyield * walkDirectory(fullPath)toyield* walkDirectory(fullPath)ensures consistent generator delegation syntax throughout the file.src/channel/set.ts (5)
6-6: New imports for interactive prompts
ImportingisCancelandselectfrom@clack/promptshelps provide a more interactive user flow. This change looks straightforward and safe.
111-118: IntroducingpublicChannelstate
DeclaringpublicChanneland equating'public'or'default'with a booleantrueseems functional. However, it could be confusing to treat"public"and"default"identically. If future requirements call for differentiating them, consider clarifying or splitting the logic.
153-153: Setting the channel in the database
This line updates the channel by callingupdateOrCreateChannel(...). Looks consistent with the overarching logic. Just ensure callers have the necessary permissions to avoid partial failures.
250-252: Public/private logging
Logging the final channel state when only one of iOS or Android is made “public” is user-friendly. The code is consistent with the rest of the logic.
270-271: Enhanced error context
Providing more context about the channel and app ID in error logs is helpful for debugging. This addition improves maintainability and clarity of error messages.src/api/channels.ts (2)
144-164: Leverage concurrency effectively and confirm partial-error handling logic
UsingPromise.allhere is a good approach to parallelize the two queries and reduce total query time. Ensure that if one query fails, the other’s successful result is not used inappropriately. The downstream checks at lines 171–173 appear to handle this, but do confirm that partial-result scenarios won’t cause edge-case issues.
176-181: Verify overshadowing ofpublicfield
You’re returning a computedpublicproperty that overrides the database row’spubliccolumn. Confirm whether this is intentional or if the table column should be dropped/renamed to avoid confusion now that “private” usage is removed.src/types/supabase.types.ts (12)
10-34: Newgraphql_publicschema block
This addition looks consistent for defining GraphQL schema methods. Confirm you have tests or typed queries to validate and leverage thegraphqlfunction.
233-235: Check nullability ondefault_channel_sync
Unlikedefault_channel_androidanddefault_channel_ios, which allow null,default_channel_syncis strictly boolean. Consider if you need a default or if it should also accept null to accommodate older rows or optional usage.
250-252: Insertion logic for default channel fields
Having these extra fields is fine, but ensure your backend or migrations supply a sensible default if clients omit them.
267-269: Update logic for default channel fields
Similar to insertion, confirm any existing rows can handle these new nullable or boolean fields during updates.
282-295: Relationships tochannels
The foreign key definitions referencingchannelslook well-defined. Confirm they are intentionally not one-to-one if multiple apps can share the same default channel.
568-568: Non-nullabledeleted_account
If this table previously stored null emails, this change can break insertion logic. Verify the data or ensure the field is always populated.
960-960: Introducingversiontoplansrow
Storing a numeric version can work, but confirm it aligns with your application’s versioning scheme (e.g., semantic versioning might require strings).
983-983: OptionalversioninInsert
Adding this optional field is valid. Just be sure the database can handle an unset version gracefully.
1006-1006: OptionalversioninUpdate
Similar to insertion, confirm partial updates on existing records handle the new version attribute.
1399-1410: New overload returningdeleted_appsinget_app_metrics
Great addition. Check that downstream consumers or type checks handle the new shape correctly if existing code doesn’t expectdeleted_appsdata.
2148-2156: Newreplicate_to_d1function
This function can help keep data in sync. Ensure you handle potential partial failures ifd1is unavailable.
2281-2281: Newusage_modeenum
Expanding enumerations can be useful, but confirm older references or queries don’t assume a smaller set of options.src/bundle/upload.ts (2)
470-479: Good addition of app data retrieval for default channels.This code correctly fetches the default channel identifiers from the apps table, which is part of removing the 'private' field usage from the channel table. The error handling is also well implemented.
495-495: Appropriate logic update for public channel detection.This change now determines if a channel is public by checking if it matches either the default iOS or Android channel, rather than using a 'private' property. This aligns well with the PR objective of removing the 'private' field usage.
| log.error(`App ${appid} not found in database`) | ||
| console.log(vError, appError) |
There was a problem hiding this comment.
Use allowed logging methods to fix pipeline error
ESLint forbids console.log; replace it with log.warn or log.error to comply with your logging guidelines and stop the pipeline failure.
- console.log(vError, appError)
+ log.error(vError, appError)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log(vError, appError) | |
| log.error(vError, appError) |
🧰 Tools
🪛 ESLint
[error] 173-173: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
🪛 GitHub Actions: Run tests
[error] 173-173: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
🪛 GitHub Actions: autofix.ci
[error] 173-173: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/api/channels.ts (1)
180-184:⚠️ Potential issueReplace console.log with log.error to fix pipeline error.
ESLint forbids
console.log; replace it withlog.errorto comply with your logging guidelines and fix the pipeline failure.if (vError || appError) { log.error(`App ${appid} not found in database`) - console.log(vError, appError) + log.error(vError, appError) program.error('') }🧰 Tools
🪛 ESLint
[error] 182-182: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
🪛 GitHub Actions: Run tests
[error] 182-182: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
🪛 GitHub Actions: autofix.ci
[error] 182-182: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
🧹 Nitpick comments (1)
src/channel/set.ts (1)
272-282: Consider adding validation for platform-specific visibility constraints.When setting a channel as public for a single platform, consider checking if there's already a different public channel for that platform. This would prevent unexpected behavior where multiple channels are marked as default for the same platform.
const opositePlatform = platform === 'iOS' ? 'android' : 'ios' +const currentPlatformDefault = appData?.[`default_channel_${platform.toLowerCase()}`] + +// Check if there's already a different public channel for this platform +if (currentPlatformDefault && currentPlatformDefault !== channelData.id) { + const confirm = await confirmC({ + message: `Channel '${channel}' will replace the current default ${platform} channel. Continue?` + }) + if (!confirm) { + outro(`Operation cancelled`) + exit() + } +} const { error: singlePlatformError } = await supabase
📜 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 (4)
src/api/channels.ts(2 hunks)src/bundle/upload.ts(2 hunks)src/channel/set.ts(5 hunks)src/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bundle/upload.ts
- src/utils.ts
🧰 Additional context used
🪛 ESLint
src/api/channels.ts
[error] 182-182: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
🪛 GitHub Actions: Run tests
src/api/channels.ts
[error] 182-182: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
🪛 GitHub Actions: autofix.ci
src/api/channels.ts
[error] 182-182: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error (no-console)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check-posix-paths-macos
🔇 Additional comments (6)
src/api/channels.ts (2)
153-178: Good use of Promise.all for concurrent API calls.The refactoring to use Promise.all for parallel execution of multiple queries improves performance by reducing wait time. This approach is more efficient than sequential queries, especially when the results are independent.
185-190: Good approach to computing the public property dynamically.Computing the
publicproperty based on whether the channel matches one of the default channels is a clean approach. This eliminates the need for a separate database field and ensures consistency.src/channel/set.ts (4)
131-131: Good initialization pattern for tracking channel publicity.Using a nullable boolean to track the channel's publicity state separately from the payload is a clean approach.
155-156: Consistent with updated public flag approach.This change aligns with the updated approach for determining channel publicity based on default channel settings rather than a direct public flag.
204-298: Well-structured interactive flow for managing channel publicity.The implementation handles various scenarios well:
- Clearing default status when a channel is made private
- Detecting if a channel is already public
- Interactive selection for platform-specific publicity
- Proper error handling for database operations
The use of the select prompt creates a good user experience for managing platform-specific settings.
316-318: Improved error handling with detailed error messages.The enhanced error message provides context about which channel and app ID were involved, making debugging easier.
riderx
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores