Feat: Handler time complexity reduction #66
Conversation
📝 WalkthroughWalkthroughIntroduce hostname metadata to DeepLinkHandler and register handlers in a hostname->handler Map; generateDeepLink now resolves handlers by extracted hostname (with SLD heuristics) and logs collisions; platform handlers gain hostnames arrays; Zoom regex updated to include zoom.com subdomains. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant getHostname
participant HandlerMap
participant Handler
participant UnknownHandler
Client->>getHostname: provide URL
getHostname-->>Client: normalized hostname
Client->>HandlerMap: lookup(hostname)
alt handler found
HandlerMap-->>Client: handler
Client->>Handler: handler.match(url) / handler.build(...)
Handler-->>Client: deep link result
else no handler
HandlerMap-->>Client: none
Client->>UnknownHandler: fallback handling
UnknownHandler-->>Client: unknown result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/core/src/platforms/github.ts (1)
4-4: LGTM! Hostnames correctly reflect the match pattern.The hostnames array accurately lists both
github.comandwww.github.com, which aligns perfectly with the optionalwww.prefix in the regex pattern (line 7). This enables the O(1) handler lookup optimization.Optional suggestion for maintainability:
Consider extracting the hostnames to a constant at the top of the file to reduce duplication and make it easier to maintain consistency with the regex pattern:
🔎 Optional refactor
import { DeepLinkHandler, Platform } from '../types'; +const GITHUB_HOSTNAMES = ['github.com', 'www.github.com'] as const; + export const githubHandler: DeepLinkHandler = { - hostnames: ['github.com', 'www.github.com'], + hostnames: GITHUB_HOSTNAMES, match: (url) => url.match( /^https?:\/\/(?:www\.)?github\.com\/([^\/?#]+)\/?([^\/?#]+)?\/?([^\/?#]+)?\/?([^\/?#]+)?/, ),packages/core/src/index.ts (2)
33-39: Consider detecting duplicate hostnames during initialization.If two handlers accidentally register the same hostname,
Map.setwill silently overwrite the first handler with the second. This could lead to unexpected behavior where the wrong handler processes URLs for that hostname.Consider adding validation to detect and warn about duplicate hostnames during map construction for more robust error handling.
💡 Example validation approach
handlers.forEach((handler) => { handler.hostnames.forEach((hostname) => { + if (handlerMap.has(hostname)) { + console.warn(`Duplicate hostname detected: ${hostname} is registered by multiple handlers`); + } handlerMap.set(hostname, handler); }); });
41-48: LGTM!The
getHostnamehelper correctly normalizes URLs by prepending"https://"when needed and extracts the hostname using the URL API, which also handles lowercase normalization automatically.The error handling appropriately returns
nullfor invalid URLs, allowing the caller to fall back tounknownHandler.Optional: Consider logging parse errors for debugging
Silent error handling could make debugging harder if URLs unexpectedly fail to parse. Consider logging errors in development:
function getHostname(url: string): string | null { try { const urlWithProtocol = url.startsWith('http') ? url : `https://${url}`; return new URL(urlWithProtocol).hostname; } catch (e) { + if (process.env.NODE_ENV === 'development') { + console.debug(`Failed to parse URL: ${url}`, e); + } return null; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/core/src/index.tspackages/core/src/platforms/discord.tspackages/core/src/platforms/facebook.tspackages/core/src/platforms/github.tspackages/core/src/platforms/instagram.tspackages/core/src/platforms/linkedin.tspackages/core/src/platforms/reddit.tspackages/core/src/platforms/spotify.tspackages/core/src/platforms/threads.tspackages/core/src/platforms/twitch.tspackages/core/src/platforms/whatsapp.tspackages/core/src/platforms/youtube.tspackages/core/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/index.ts (2)
packages/core/src/types.ts (2)
DeepLinkHandler(27-50)DeepLinkResult(15-20)packages/core/src/platforms/reddit.ts (1)
redditHandler(3-27)
🔇 Additional comments (13)
packages/core/src/platforms/spotify.ts (1)
4-4: LGTM! Hostnames declaration aligns with the regex pattern.The
hostnamesarray correctly declaresopen.spotify.com, which matches the hostname pattern in the regex at line 7.packages/core/src/types.ts (1)
28-32: LGTM! Type extension enables the O(1) optimization.The
hostnamesfield addition to theDeepLinkHandlerinterface enables hostname-based dispatch as described in the PR objectives. The comment clearly documents its purpose.packages/core/src/platforms/threads.ts (1)
4-5: LGTM! Hostnames declaration aligns with the regex pattern.The
hostnamesarray correctly declares boththreads.netandwww.threads.net, matching the optionalwwwsubdomain in the regex pattern at line 5.packages/core/src/platforms/twitch.ts (1)
23-23: LGTM! Comprehensive hostname coverage for Twitch.The hostnames list correctly includes the main domain, www variant, mobile subdomain, and clips subdomain, properly supporting all URL patterns defined in the PATTERNS array.
packages/core/src/platforms/discord.ts (1)
4-4: LGTM! Hostname list correctly covers Discord domains.The hostnames include both discord.com (with www variant) and discord.gg, properly supporting all patterns in the match regex for channels and invites.
packages/core/src/platforms/whatsapp.ts (1)
4-4: LGTM! WhatsApp hostnames correctly defined.The hostnames list properly covers wa.me with and without www prefix, aligning with the match regex.
packages/core/src/platforms/youtube.ts (1)
32-32: LGTM! Comprehensive YouTube hostname coverage.The hostnames list correctly includes youtube.com variants (standard, www, mobile) and the official short URL domain youtu.be, properly supporting all URL patterns handled by extractYouTubeVideoId.
packages/core/src/platforms/facebook.ts (1)
4-4: Consider documenting the intended scope of Facebook URL handling.The hostnames list currently covers the primary Facebook domains (facebook.com, www.facebook.com, m.facebook.com). While short URL services like fb.com, fb.watch, and fb.me exist, they are not referenced anywhere in the codebase and the regex pattern only matches facebook.com variants. If these short domains are intentionally out of scope, consider adding a comment to clarify what URLs this handler is designed to support.
packages/core/src/index.ts (3)
13-15: LGTM!The additional imports for
redditHandlerandDeepLinkHandlertype are correctly added to support the new typed handler array and reddit platform functionality.
19-31: LGTM!Explicitly typing the
handlersarray asDeepLinkHandler[]provides compile-time validation that all handlers implement the required interface, including the newhostnamesfield essential for the O(1) lookup optimization.
50-65: Excellent refactoring – achieves the O(1) lookup objective!The refactored
generateDeepLinksuccessfully replaces the previous O(n) handler iteration with O(1) map-based dispatch. The control flow correctly handles all edge cases:
- Invalid URL → falls back to
unknownHandler- Hostname not registered → falls back to
unknownHandler- Handler found but pattern doesn't match → falls back to
unknownHandler- Handler found and pattern matches → builds deep link
This optimization eliminates redundant handler matching on every link click, running the iteration cost only once at initialization as intended.
packages/core/src/platforms/linkedin.ts (2)
14-41: Nice formatting improvements.The inline arrow functions and consistent semicolon usage improve code readability without altering functionality.
48-61: Match and build logic preserved correctly.The formatting changes maintain the existing match-and-build pattern without introducing functional changes. The custom match array structure works correctly with the build function.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/platforms/linkedin.ts (1)
47-47: Missingwww.linkedin.comhostname will break most LinkedIn URLs.The hostnames array only contains
'linkedin.com', but most LinkedIn URLs usewww.linkedin.com. SincegetHostnameextractswww.linkedin.comfrom URLs likehttps://www.linkedin.com/in/profile, the handlerMap lookup will fail and fall back tounknownHandler.At minimum, add
www.linkedin.com. Consider also adding country-specific subdomains (in.linkedin.com,fr.linkedin.com, etc.) as noted in prior review.Suggested fix
- hostnames: ['linkedin.com'], + hostnames: ['linkedin.com', 'www.linkedin.com'],
🧹 Nitpick comments (1)
packages/core/src/index.ts (1)
40-68: Reasonable heuristic, but consider documenting limitations.The root domain extraction heuristic works well for the currently supported platforms. However, it may produce incorrect results for some edge cases:
amazon.co.uk→ correctly extractsamazon.co.ukexample.com.br→ correctly handles second-level TLDs- But domains like
blogspot.co.ukwould extractblogspot.co.ukwhenexample.blogspot.co.ukmight need different handlingFor a more robust solution, consider using the Public Suffix List via a library like
psl. However, this is optional since the current approach handles all supported platforms correctly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/core/src/index.tspackages/core/src/platforms/discord.tspackages/core/src/platforms/facebook.tspackages/core/src/platforms/github.tspackages/core/src/platforms/instagram.tspackages/core/src/platforms/linkedin.tspackages/core/src/platforms/reddit.tspackages/core/src/platforms/spotify.tspackages/core/src/platforms/threads.tspackages/core/src/platforms/twitch.tspackages/core/src/platforms/whatsapp.tspackages/core/src/platforms/youtube.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/platforms/facebook.ts
- packages/core/src/platforms/discord.ts
- packages/core/src/platforms/whatsapp.ts
- packages/core/src/platforms/instagram.ts
- packages/core/src/platforms/twitch.ts
- packages/core/src/platforms/youtube.ts
- packages/core/src/platforms/reddit.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/index.ts (1)
packages/core/src/types.ts (2)
DeepLinkHandler(27-50)DeepLinkResult(15-20)
🔇 Additional comments (3)
packages/core/src/platforms/github.ts (1)
3-4: LGTM!The hostname
github.comis correctly specified. SincegetHostnameextracts the root domain (last 2 parts), URLs withwww.github.comwill normalize togithub.comand match correctly.packages/core/src/platforms/spotify.ts (1)
3-4: LGTM!The hostname
spotify.comcorrectly covers URLs fromopen.spotify.comsincegetHostnameextracts the root domain.packages/core/src/index.ts (1)
70-85: LGTM!The dispatch logic correctly implements O(1) handler lookup with appropriate fallbacks. The flow is clear: extract hostname → lookup handler → attempt match → build result or fall back to unknown.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/index.ts (1)
41-69: Consider using a Public Suffix List for more robust domain extraction.The current heuristic handles common cases well (e.g.,
.co.uk,.com.br), but may fail on rare multi-level TLDs like.pvt.k12.ma.us. For production use with diverse URLs, consider using a library likepsl(Public Suffix List) for definitive root-domain extraction.Given that the comment acknowledges this is "naive" and the target platforms (social media, streaming) typically use standard TLDs, the current implementation is acceptable as a starting point.
Alternative: Use the `psl` library
import psl from 'psl'; function getHostname(url: string): string | null { try { const urlWithProtocol = url.startsWith('http') ? url : `https://${url}`; const hostname = new URL(urlWithProtocol).hostname; const parsed = psl.parse(hostname); return parsed.domain || hostname; } catch (e) { return null; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/index.ts
🔇 Additional comments (3)
packages/core/src/index.ts (3)
13-13: Previous issue resolved:redditHandlernow properly integrated.The
redditHandleris now correctly added to both the imports and thehandlersarray (line 30), resolving the previous review concern.
19-31: LGTM: Handlers array properly typed and complete.The array is now explicitly typed as
DeepLinkHandler[]and includes all platform handlers, including the previously missingredditHandler.
71-86: LGTM: Handler dispatch correctly implements O(1) lookup.The refactored logic successfully achieves the PR objective by using hostname-based map lookup instead of iterating all handlers. The fallback chain (hostname → handler → match → unknownHandler) is logically sound with proper null safety.
|
Hii @mdsaban, I have resolved all the issues raised by CodeRabbitAI above. Now it's safe to merge. |
|
Hii @mdsaban, I see the PR is still pending to merge. Did you find any issues with it? If so, please let me know, I'd be happy to resolve it! |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/index.ts (1)
84-99: Critical: URL normalization bug breaks protocol-less URLs.The
getHostnamefunction (Line 86) addshttps://protocol internally for hostname extraction (see Line 56), butgenerateDeepLinkpasses the originalwebUrl(without protocol if missing) tohandler.match(webUrl)on Line 91.Handlers with regexes that require protocol (e.g., zoom.ts Line 23:
/^https?:\/\/.../, substack.ts Line 20) will fail to match protocol-less URLs, even though the hostname lookup succeeds.Impact: If a user provides
zoom.us/j/1234567890, the hostname lookup findszoomHandler, but the match fails because the regex expectshttps://zoom.us/j/1234567890.🔧 Proposed fix: Normalize URL before matching
export function generateDeepLink(url: string): DeepLinkResult { - const webUrl = url.trim(); + // Normalize URL by adding protocol if missing + const trimmedUrl = url.trim(); + const webUrl = trimmedUrl.startsWith('http') ? trimmedUrl : `https://${trimmedUrl}`; const hostname = getHostname(webUrl); if (hostname) { const handler = handlerMap.get(hostname); if (handler) { const match = handler.match(webUrl); if (match) { return handler.build(webUrl, match); } } } return unknownHandler(webUrl); }This ensures the protocol-normalized URL is consistently used for both matching and building deep links.
🤖 Fix all issues with AI agents
In @packages/core/src/index.ts:
- Around line 43-52: The code currently warns on hostname collisions in the
handlers.forEach / handler.hostnames.forEach loop but then unconditionally
overwrites the previous mapping in handlerMap.set(hostname, handler); change
this to preserve the first-registered handler by skipping the set when
handlerMap.has(hostname) is true (i.e., only call handlerMap.set when the
hostname is not present) and update the warning to include which handler was
ignored (use handler and handlerMap.get(hostname) to report the conflict);
alternatively, if you want fail-fast behavior, replace the warning with throwing
an Error to prevent silent changes — choose one policy and implement it in the
loop where handlerMap and handler.hostnames are used.
In @packages/core/src/platforms/telegram.ts:
- Line 4: Update the Telegram domain list and regex: remove 'telegram.com' from
the hostnames array (hostnames) and add 'telegram.dog' to that array, then
update the match regular expression (the match variable/regex that currently
handles t.me and telegram.me) to also accept telegram.dog and not include
telegram.com so hostname matching aligns with official Telegram domains.
In @packages/core/src/platforms/zoom.ts:
- Line 18: The hostnames array lists 'zoom.com' but the match regex in the Zoom
platform handler only matches zoom.us, causing a mismatch; update the handler so
the match regular expression used by the Zoom platform (the variable or property
named match) accepts both zoom.us and zoom.com domains (e.g., allow either
domain in the host portion of the pattern), or alternatively remove 'zoom.com'
from the hostnames array if that domain should not be supported—ensure the
change keeps the same capture groups/intent of the existing regex and that
hostnames and match remain consistent.
🧹 Nitpick comments (1)
packages/core/src/index.ts (1)
54-82: Root domain extraction heuristic may not cover all TLD variations.The heuristic for second-level TLDs (Lines 65-72) checks if the TLD is 2 characters and the SLD is ≤ 4 characters. While this covers common cases like
.co.uk, it may not handle all international TLD patterns (e.g.,.com.br,.gov.au).The comment on Line 59 acknowledges this as "naive," which is appropriate. For the current use case with major platforms, this should be sufficient, but consider documenting known limitations or edge cases.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/index.tspackages/core/src/platforms/pinterest.tspackages/core/src/platforms/snapchat.tspackages/core/src/platforms/substack.tspackages/core/src/platforms/telegram.tspackages/core/src/platforms/zoom.tspackages/core/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/types.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tanmoyPaul12
Repo: mdsaban/universal-app-opener PR: 0
File: :0-0
Timestamp: 2026-01-02T21:27:30.363Z
Learning: In the mdsaban/universal-app-opener repository, the Platform type in packages/core/src/types.ts already includes 'github' and 'twitch' platforms in the main branch.
📚 Learning: 2026-01-02T21:27:30.363Z
Learnt from: tanmoyPaul12
Repo: mdsaban/universal-app-opener PR: 0
File: :0-0
Timestamp: 2026-01-02T21:27:30.363Z
Learning: In the mdsaban/universal-app-opener repository, the Platform type in packages/core/src/types.ts already includes 'github' and 'twitch' platforms in the main branch.
Applied to files:
packages/core/src/index.ts
🔇 Additional comments (3)
packages/core/src/platforms/pinterest.ts (1)
67-67: LGTM!The
hostnamesfield correctly declares'pinterest.com', which aligns with the match patterns defined on Lines 5-9.packages/core/src/platforms/substack.ts (1)
17-17: LGTM!The
hostnamesfield correctly declares'substack.com'. The root domain extraction logic inindex.tswill map any*.substack.comsubdomain to this handler, and the match regex on Line 20 properly handles the subdomain pattern.packages/core/src/platforms/snapchat.ts (1)
4-4: The hostnames addition aligns with the PR objectives for O(1) handler lookup.The hostname extraction logic in
getHostname()automatically normalizes subdomains by extracting the root domain (last 2 parts), sohttps://www.snapchat.com/add/usercorrectly matches the handler registered undersnapchat.com. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/index.ts (1)
55-83: Consider Public Suffix List for robust TLD handling.The hostname extraction logic uses a heuristic to handle second-level TLDs (e.g.,
.co.uk). While this works for common cases, it has limitations:
- Current heuristic: Checks if TLD is 2 chars AND SLD is ≤ 4 chars
- May fail on: Complex cases like
.pvt.k12.ma.usor uncommon TLD combinations- Works well for: Common domains like
youtube.com,github.com,example.co.uk,example.com.auFor the current use case (deep linking to popular platforms), this heuristic is likely sufficient.
If you want to handle all edge cases, consider using a Public Suffix List library:
// Example using a PSL library (would need to add dependency) import { parse } from 'psl'; // or similar library function getHostname(url: string): string | null { try { const urlWithProtocol = url.startsWith('http') ? url : `https://${url}`; const hostname = new URL(urlWithProtocol).hostname; const parsed = parse(hostname); return parsed.domain || hostname; // Returns registered domain } catch (e) { return null; } }However, this adds a dependency. The current heuristic is acceptable given the project's scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T21:27:30.363Z
Learnt from: tanmoyPaul12
Repo: mdsaban/universal-app-opener PR: 0
File: :0-0
Timestamp: 2026-01-02T21:27:30.363Z
Learning: In the mdsaban/universal-app-opener repository, the Platform type in packages/core/src/types.ts already includes 'github' and 'twitch' platforms in the main branch.
Applied to files:
packages/core/src/index.ts
🔇 Additional comments (4)
packages/core/src/index.ts (4)
20-20: LGTM: Type safety improvements.The explicit import of
DeepLinkHandlerand type annotation for thehandlersarray improve type safety and enable the new handlerMap initialization.Also applies to: 25-25
48-51: Verify collision handling behavior is intentional.When multiple handlers claim the same hostname, the current implementation logs a warning but allows the last handler to overwrite previous ones (line 51). This could lead to non-deterministic behavior depending on handler registration order.
Consider whether collision should:
- Throw an error (fail-fast during initialization)
- Keep the first handler (first-wins strategy)
- Current behavior: keep the last handler (last-wins strategy)
Please confirm the intended collision resolution strategy. If last-wins is intentional, consider adding a comment explaining this behavior.
87-97: LGTM: Successfully achieves O(1) handler lookup.The refactored
generateDeepLinknow:
- Extracts hostname in O(1) time
- Performs O(1) map lookup instead of O(n) iteration
- Maintains proper fallback behavior to
unknownHandlerThis successfully achieves the PR objective of eliminating the per-link O(n) handler search. The change in behavior (only one handler per hostname) is more deterministic than the previous sequential matching approach.
46-53: TypeScript types already guaranteehostnamesproperty exists.The
handlersarray is typed asDeepLinkHandler[], and theDeepLinkHandlerinterface defineshostnames: string[]as a required (non-optional) property. TypeScript compile-time type checking ensures all handlers in the array have this property, making runtime validation unnecessary.Likely an incorrect or invalid review comment.
|
Hi @mdsaban. I have resolved the merge conflict issues. Can you please take a look at the PR? Thanks! |
1 participant
With this change, we implement an
O(1)solution for finding handlers, utilizingO(n)complexity only during initialization. Previously, the handler finding loop was re-executed on every link click; now, the loop executes once, and subsequent lookups are optimized.This is implemented by adding valid
hostNamesin each and every handler file.Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.