Fix bluesky crash + username check #475
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
@divyaswormakai is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request enhance error handling and user experience across several components. In the Changes
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (3)
apps/frontend/src/app/(site)/integrations/social/[provider]/continue/page.tsx (1)
34-37: Consider enhancing error message handling.The error message is directly used in the URL without encoding, which could cause issues with special characters.
- return redirect(`/launches?msg=${msg}`); + return redirect(`/launches?msg=${encodeURIComponent(msg)}`);libraries/nestjs-libraries/src/integrations/social/bluesky.provider.ts (2)
72-110: Consider enhancing error response structure.While the error handling is good, the empty response structure might not provide enough context to the frontend about what went wrong.
Consider adding an error field to the response:
return { refreshToken: '', expiresIn: 0, accessToken: '', id: '', name: '', picture: '', username: '', + error: error.message || 'Authentication failed' };
198-200: Simplify URL construction.The current implementation of splitting and popping could be simplified.
- releaseURL: `https://bsky.app/profile/${id}/post/${cidUrl[index].url - .split('/') - .pop()}`, + releaseURL: `https://bsky.app/profile/${id}/post/${cidUrl[index].url.split('/').at(-1)}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/frontend/src/app/(site)/integrations/social/[provider]/continue/page.tsx(1 hunks)apps/frontend/src/components/launches/add.provider.component.tsx(3 hunks)libraries/nestjs-libraries/src/integrations/social/bluesky.provider.ts(5 hunks)
🔇 Additional comments (3)
apps/frontend/src/app/(site)/integrations/social/[provider]/continue/page.tsx (1)
79-92: LGTM! Comprehensive error handling implementation.
The try-catch block with fallback UI effectively prevents application crashes and provides a good user experience.
libraries/nestjs-libraries/src/integrations/social/bluesky.provider.ts (1)
33-34: LGTM! Improved input validation and user guidance.
The placeholder and validation regex improvements help users understand the expected format.
apps/frontend/src/components/launches/add.provider.component.tsx (1)
193-193: LGTM! Enhanced form input experience.
The addition of placeholder support improves user guidance across the component hierarchy.
Also applies to: 275-275, 319-319
jamesread
left a comment
jamesread
left a comment
There was a problem hiding this comment.
Hey @divyaswormakai , thanks so much indeed for taking the time to do this, good idea, and generally good PR. I left a couple of comments for you to address, hope that's OK!
apps/frontend/src/app/(site)/integrations/social/[provider]/continue/page.tsx
Outdated
Show resolved
Hide resolved
apps/frontend/src/app/(site)/integrations/social/[provider]/continue/page.tsx
Outdated
Show resolved
Hide resolved
apps/frontend/src/app/(site)/integrations/social/[provider]/continue/page.tsx
Outdated
Show resolved
Hide resolved
libraries/nestjs-libraries/src/integrations/social/bluesky.provider.ts
Outdated
Show resolved
Hide resolved
libraries/nestjs-libraries/src/integrations/social/bluesky.provider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
libraries/helpers/src/integrations/bluesky.provider.ts (5)
1-21: LGTM! Consider improving comment formatting.The documentation is comprehensive and well-attributed. Consider converting this into JSDoc format for better IDE integration.
-// Handle constraints, in English: -// - must be a possible domain name +/** + * Handle constraints, in English: + * - must be a possible domain name + * ... + */
22-54: Consider using custom error types for better error handling.The implementation is thorough but could be improved by using custom error types to allow consumers to handle specific validation failures differently.
+export class HandleValidationError extends Error { + constructor(message: string) { + super(message); + this.name = 'HandleValidationError'; + } +} export const ensureValidHandle = (handle: string): void => { if (!/^[a-zA-Z0-9.-]*$/.test(handle)) { - throw new Error( + throw new HandleValidationError( 'Disallowed characters in handle (ASCII letters, digits, dashes, periods only)' ); } // ... similar changes for other error throws
56-68: Improve regex maintainability with documentation and named constants.The regex pattern is complex and could be hard to maintain. Consider breaking it down into named constants and adding documentation.
+const HANDLE_SEGMENT = '[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?'; +const TLD_SEGMENT = '[a-zA-Z]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?'; +/** + * Validates handle format using regex: + * - Segments must start and end with alphanumeric + * - Middle can contain hyphens + * - TLD must start with a letter + */ export const ensureValidHandleRegex = (handle: string): void => { if ( - !/^([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?$/.test( + !new RegExp(`^(${HANDLE_SEGMENT}\\.)+${TLD_SEGMENT}$`).test( handle ) ) {
74-78: Consider wrapping validation errors with context.The function could provide more context when validation fails by including the original handle in the error message.
export const normalizeAndEnsureValidHandle = (handle: string): string => { const normalized = normalizeHandle(handle); - ensureValidHandle(normalized); + try { + ensureValidHandle(normalized); + } catch (err) { + throw new Error(`Invalid handle "${handle}": ${err.message}`); + } return normalized; };
1-92: Implementation aligns well with PR objectives.The handle validation implementation provides a robust foundation for preventing crashes from invalid Bluesky credentials. The multiple validation approaches (detailed and regex-based) offer flexibility while maintaining strict validation rules.
Consider the following architectural improvements:
- Add rate limiting to prevent DoS attacks through expensive regex operations
- Consider caching validated handles to improve performance
- Add metrics/monitoring for validation failures to track potential abuse
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/frontend/src/app/(site)/integrations/social/[provider]/continue/page.tsx(2 hunks)libraries/helpers/src/integrations/bluesky.provider.ts(1 hunks)libraries/nestjs-libraries/src/integrations/social/bluesky.provider.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/frontend/src/app/(site)/integrations/social/[provider]/continue/page.tsx
- libraries/nestjs-libraries/src/integrations/social/bluesky.provider.ts
🔇 Additional comments (1)
libraries/helpers/src/integrations/bluesky.provider.ts (1)
70-72: LGTM!
Simple and correct implementation of case normalization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
libraries/helpers/src/integrations/bluesky.provider.ts (2)
1-21: Add JSDoc documentation for exported functions.While the handle constraints are well documented, adding JSDoc comments with examples would improve the API documentation.
Add JSDoc comments like this:
+/** + * Validates a Bluesky handle against domain name constraints. + * @param handle - The handle to validate + * @throws {Error} If the handle is invalid + * @example + * // Valid handles + * ensureValidHandle('alice.bsky.social') + * ensureValidHandle('bob-123.test.com') + * + * // Invalid handles + * ensureValidHandle('alice') // Throws: Handle domain needs at least two parts + * ensureValidHandle('-alice.com') // Throws: Handle parts can not start or end with hyphens + */ export const ensureValidHandle = (handle: string): void => {
22-54: Extract magic numbers into named constants.The function implementation is robust, but readability could be improved by extracting magic numbers into named constants.
Consider this refactoring:
+const HANDLE_CONSTRAINTS = { + MAX_TOTAL_LENGTH: 253, + MAX_LABEL_LENGTH: 63, + MIN_LABELS: 2, + VALID_CHARS_PATTERN: /^[a-zA-Z0-9.-]*$/, + TLD_START_PATTERN: /^[a-zA-Z]/ +} as const; + export const ensureValidHandle = (handle: string): void => { + if (handle == null) { + throw new Error('Handle cannot be null or undefined'); + } + // check that all chars are boring ASCII - if (!/^[a-zA-Z0-9.-]*$/.test(handle)) { + if (!HANDLE_CONSTRAINTS.VALID_CHARS_PATTERN.test(handle)) { throw new Error( 'Disallowed characters in handle (ASCII letters, digits, dashes, periods only)' ); } - if (handle.length > 253) { + if (handle.length > HANDLE_CONSTRAINTS.MAX_TOTAL_LENGTH) { throw new Error('Handle is too long (253 chars max)'); } const labels = handle.split('.'); - if (labels.length < 2) { + if (labels.length < HANDLE_CONSTRAINTS.MIN_LABELS) { throw new Error('Handle domain needs at least two parts'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
libraries/helpers/src/integrations/bluesky.provider.ts(1 hunks)
🔇 Additional comments (3)
libraries/helpers/src/integrations/bluesky.provider.ts (3)
70-72: LGTM!
The function correctly implements case normalization as specified in the handle constraints.
74-78: LGTM!
The function follows a good pattern of normalizing before validation and correctly returns the normalized handle.
80-88: Improve error handling and logging.
The current implementation uses console.log which isn't suitable for production code.
As suggested in the previous review, consider using the NestJS Logger:
+import { Logger } from '@nestjs/common';
+
export const isValidHandle = (handle: string): boolean => {
+ const logger = new Logger('BlueskyProvider');
try {
ensureValidHandle(handle);
return true;
} catch (err) {
- console.log('Error in bluesky handle validation:', err);
+ logger.debug(`Handle validation failed: ${err instanceof Error ? err.message : 'Unknown error'}`);
return false;
}
};
|
@jamesread updated the code likewise :) |
|
Thank you so much for this PR, but I feel it's a bit over-coded, I pushed a very simplistic implementation for this. |
3 participants
What kind of change does this PR introduce?
Fixes #470
Fixes app crashing when invalid bluesky credentials are provided.
Why was this change needed?
Added try catch block for the code on frontend and backend to stop applications from crashing.
Other information:
Checklist:
Put a "X" in the boxes below to indicate you have followed the checklist;
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes