-
Notifications
You must be signed in to change notification settings - Fork 65
Added Twitter profile #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Added Twitter profile #18
Conversation
📝 WalkthroughWalkthroughAdds Twitter/X deep-link support: new twitter handler, registered in core, Platform type extended, and demo UI updated with an example link and hover styles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Demo as Demo UI
participant Core as DeepLink Core
participant Handler as twitterHandler
participant OS as Mobile OS
Note over Demo,Core: User clicks "Twitter Profile" example
Demo->>Core: request deep link for webUrl (x.com/username)
Core->>Handler: iterate handlers -> match(webUrl)
Handler-->>Core: match success + extracted username
Core->>Handler: build(webUrl, match)
Handler-->>Core: returns payload (webUrl, ios, android, platform)
Core-->>Demo: select best deep link (ios/android/web) based on environment
Demo->>OS: open chosen deep link (intent or scheme) or fallback to web
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (2)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/index.ts (1)
1-1: Minor formatting: Add space after comma.The import statement is missing a space after the comma before
twitterHandler.🔎 Proposed fix
-import { instagramHandler, linkedinHandler, unknownHandler, youtubeHandler,twitterHandler } from "./platforms"; +import { instagramHandler, linkedinHandler, unknownHandler, youtubeHandler, twitterHandler } from "./platforms";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/demo/index.html(1 hunks)apps/demo/src/style.css(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/platforms/index.ts(1 hunks)packages/core/src/platforms/twitter.ts(1 hunks)packages/core/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/platforms/twitter.ts (2)
packages/core/src/platforms/index.ts (1)
twitterHandler(12-12)packages/core/src/types.ts (1)
DeepLinkHandler(10-13)
packages/core/src/index.ts (3)
packages/core/src/platforms/index.ts (4)
youtubeHandler(9-9)linkedinHandler(8-8)instagramHandler(10-10)twitterHandler(12-12)packages/core/src/platforms/instagram.ts (1)
instagramHandler(3-17)packages/core/src/platforms/twitter.ts (1)
twitterHandler(4-16)
🔇 Additional comments (4)
packages/core/src/index.ts (1)
6-11: LGTM: Handler integration looks good.The
twitterHandleris correctly added to the handlers array, following the same pattern as other platform handlers.packages/core/src/platforms/index.ts (1)
5-5: LGTM: Clean integration.The Twitter handler is properly imported and exported alongside existing platform handlers.
Also applies to: 11-12
packages/core/src/types.ts (1)
1-1: LGTM: Type definition correctly extended.The Platform union type now includes 'twitter', maintaining type safety across the codebase.
packages/core/src/platforms/twitter.ts (1)
11-12: The deep link schemes remain valid after the X rebrand.The iOS scheme
twitter://and Android packagecom.twitter.androidare still correct and haven't changed despite the Twitter-to-X rebrand. No updates needed.
| import { DeepLinkHandler } from '../types'; | ||
|
|
||
| export const twitterHandler: DeepLinkHandler = { | ||
| match: (url) => url.match(/twitter\.com\/([^/?]+)/), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add support for x.com domain.
Twitter rebranded to X and now primarily uses x.com. The current pattern only matches twitter.com, which is inconsistent with the demo example (line 33 in apps/demo/index.html) that uses x.com.
🔎 Proposed fix
- match: (url) => url.match(/twitter\.com\/([^/?]+)/),
+ match: (url) => url.match(/(?:twitter\.com|x\.com)\/([^/?]+)/),📝 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.
| match: (url) => url.match(/twitter\.com\/([^/?]+)/), | |
| match: (url) => url.match(/(?:twitter\.com|x\.com)\/([^/?]+)/), |
🤖 Prompt for AI Agents
In packages/core/src/platforms/twitter.ts around line 5, the match regex only
recognizes "twitter.com" and misses "x.com"; update the pattern to accept either
domain (and optional www) so it matches both twitter.com and x.com (for example
use a regex that alternates the host like
/(?:www\.)?(?:twitter|x)\.com\/([^/?]+)/) so existing capture group usage
remains unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/demo/src/style.css(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/platforms/twitter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/index.ts
- packages/core/src/platforms/twitter.ts
🔇 Additional comments (1)
apps/demo/src/style.css (1)
241-243: Great fix! Twitter brand color is now correct.The Twitter hover color has been successfully updated to
#1DA1F2(Twitter's official brand blue), addressing the previous feedback. This now matches the pattern established by other platform hover styles.
| .instagram-link:hover { | ||
| border-color:#833AB4; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after colon.
Line 246 has border-color:#833AB4; without a space after the colon, which is inconsistent with the spacing style used throughout the rest of the file (e.g., lines 104, 108, 242).
🔎 Proposed fix
.instagram-link:hover {
- border-color:#833AB4;
+ border-color: #833AB4;
}Additionally, while #833AB4 is a valid color from Instagram's gradient, consider using #E1306C (Instagram's signature pink/magenta) for better brand recognition, similar to how Twitter, YouTube, and LinkedIn use their primary brand colors.
Alternative color option
.instagram-link:hover {
- border-color:#833AB4;
+ border-color: #E1306C;
}📝 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.
| .instagram-link:hover { | |
| border-color:#833AB4; | |
| } | |
| .instagram-link:hover { | |
| border-color: #833AB4; | |
| } |
🤖 Prompt for AI Agents
In apps/demo/src/style.css around lines 245 to 247, the CSS declaration uses no
space after the colon and thus is inconsistent with the file style; update
`border-color:#833AB4;` to include a space after the colon (`border-color:
#833AB4;`) and, if desired for stronger Instagram brand recognition, replace the
hex with `#E1306C` (`border-color: #E1306C;`) to match other social-icon color
choices.
|
Thanks for this, could you resolve the conflicts? |
|
Yes i'll resolve.Thanks |
|
@SachiRicha can you also check discord contribution channel once? as there are multiple PRs for twitter, let's discuss it there |
|
Conflict resolved. @mdsaban FYI |
|
Since i'm new on discord.I'm unable to find your channel or group regarding this. Please help @mdsaban |
|
We're discussing here - https://discord.com/channels/1451197679739863177/1451197746785685567 |
|
Not able to enter. Can you send discord invite link |
|
Thank you so much. Joined |
This is my first open-source contribution, and I’m having a bit of difficulty understanding the code structure and overall setup. I would really appreciate your help and guidance in understanding it more clearly.
I added the Twitter handler based on my current understanding. If I’ve missed anything or if there are better approaches, please feel free to suggest improvements.
I’ve always been hesitant to start contributing to open-source projects because, whenever I look at them, I feel unsure about where to begin or how to understand the codebase. However, after watching your videos, I felt motivated to take this step. I’m not sure how it will turn out, but I’m glad I tried.
I would also love to contribute more to this project in the future. It would be really helpful if you could explain the best way to get started, such as which areas need contributions, how to pick issues, or any guidelines I should follow.
Please review my changes and share your feedback—it would be a great help for my learning and improvement.
I also created a new Discord account but couldn’t find your community to join. Could you please guide me on how to join?
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.