Sweetmantech/myc 3422 api apiartistsocialsscrape #171
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors GET handler typing to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant getAccountSocials
participant scrapeProfileUrlBatch
participant Response
Client->>Server: POST /artist/socials/scrape { artist_account_id }
Server->>Server: Validate artist_account_id
alt invalid
Server->>Response: 400 { error: "artist_account_id required" }
Response-->>Client: 400
else valid
Server->>getAccountSocials: fetch socials for account
alt fetch error
Server->>Response: 500 { error }
Response-->>Client: 500
else socials found
Server->>scrapeProfileUrlBatch: scrape batch(inputs)
scrapeProfileUrlBatch-->>Server: [ { runId?, datasetId?, error? , supported } ... ]
Server->>Response: 200 [ aggregated results ]
Response-->>Client: 200
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts (1)
36-39: Consider consistent response structure.The empty array response is inconsistent with error responses that include
{status, message}. Consider returning a structured response for consistency:if (!socials.length) { - res.json([]); + res.status(200).json({ status: "success", results: [] }); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controllers/ArtistSocialsController/getArtistSocialsHandler.ts(3 hunks)controllers/ArtistSocialsController/index.ts(1 hunks)controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts(1 hunks)routes.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts (2)
lib/supabase/getAccountSocials.ts (1)
getAccountSocials(9-64)lib/apify/scrapeProfileUrl.ts (1)
scrapeProfileUrl(54-97)
controllers/ArtistSocialsController/getArtistSocialsHandler.ts (1)
lib/supabase/getArtistSocials.ts (1)
getArtistSocials(40-146)
🔇 Additional comments (9)
controllers/ArtistSocialsController/getArtistSocialsHandler.ts (3)
1-2: LGTM: Type import and path update.The use of
typeimport forRequestHandlerfollows TypeScript best practices, and the updated import path reflects the file reorganization.
4-8: LGTM: Standardized handler type and corrected documentation.The handler now uses Express's
RequestHandlertype for consistency, and the route documentation correctly reflects the actual endpoint path.
13-51: LGTM: Explicit returns improve control flow.Adding explicit
returnstatements after each response prevents accidental fall-through and makes the control flow clearer.routes.ts (2)
10-13: LGTM: Clean barrel import pattern.The consolidated import of both handlers from
ArtistSocialsControllerfollows the established pattern and improves organization.
100-101: LGTM: Improved type safety and new scrape endpoint.Removing the
as anycast improves type safety for the GET route, and the new POST/artist/socials/scrapeendpoint is properly wired.controllers/ArtistSocialsController/index.ts (1)
1-2: LGTM: Clean barrel export.This index file follows the standard barrel export pattern, enabling cleaner imports in other files.
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts (3)
16-24: LGTM: Proper input validation.The handler correctly validates that
artist_account_idis present and is a string, returning a 400 error with a clear message if validation fails.
26-34: LGTM: Error handling for fetching socials.The handler properly checks the status from
getAccountSocialsand returns an appropriate 500 error if the fetch fails.
66-73: LGTM: Proper error handling.The catch block appropriately logs the error for debugging while returning a generic error message to the client for security.
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts
Outdated
Show resolved
Hide resolved
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts (1)
38-59: Add social identifiers to map results back to their sources.The results array contains only
runId,datasetId, anderror, with no way to determine which social account each result corresponds to. This issue was flagged in previous review comments and remains unresolved. Additionally, the type annotation usesProfileScrapeResult[]instead ofScrapeProfileResult[], losing thesupportedfield.Apply this diff to fix both issues:
+type ArtistSocialScrapeResult = { + social_id: number; + username: string | null; + profile_url: string | null; + runId: string | null; + datasetId: string | null; + error: string | null; + supported: boolean; +}; + - const results: ProfileScrapeResult[] = await Promise.all( + const results: ArtistSocialScrapeResult[] = await Promise.all( socials.map(async (social) => { const scrapeResult = await scrapeProfileUrl( social.profile_url ?? null, social.username ?? "" ); if (!scrapeResult) { return { + social_id: social.id, + username: social.username, + profile_url: social.profile_url, runId: null, datasetId: null, error: "Unsupported or missing profile URL", + supported: false, }; } return { + social_id: social.id, + username: social.username, + profile_url: social.profile_url, runId: scrapeResult.runId, datasetId: scrapeResult.datasetId, error: scrapeResult.error, + supported: scrapeResult.supported, }; }) );
🧹 Nitpick comments (2)
lib/apify/scrapeProfileUrl.ts (2)
21-23: Clarify the purpose of thesupportedfield.The
supportedfield is always set totruein all return paths (lines 82, 90, 97), making it redundant. If the intent is to indicate whether a platform is supported, consider returning{ supported: false }instead ofnullwhen no platform matches (line 71). Otherwise, if the field serves no functional purpose, consider removing it.Can you clarify the intended behavior? If
supportedshould vary based on platform availability, apply this diff:if (!platform) { - return null; + return { + runId: null, + datasetId: null, + error: "Unsupported platform", + supported: false, + }; }Otherwise, if it's always
true, the field adds no value and could be removed.
60-72: Consider eliminatingnullreturns for unsupported platforms.The function returns
nullwhen no platform is found (line 71), but with the newsupportedfield, this creates two ways to indicate lack of support:nullorsupported: false. This forces consumers to handle both cases.Consider always returning
ScrapeProfileResultand using thesupportedfield consistently:if (!platform) { - return null; + return { + runId: null, + datasetId: null, + error: "Unsupported platform", + supported: false, + }; }Then update the return type to
Promise<ScrapeProfileResult>(non-nullable) for a cleaner API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts(1 hunks)lib/apify/scrapeProfileUrl.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts (2)
lib/supabase/getAccountSocials.ts (1)
getAccountSocials(9-64)lib/apify/scrapeProfileUrl.ts (2)
ProfileScrapeResult(15-19)scrapeProfileUrl(57-100)
🔇 Additional comments (4)
lib/apify/scrapeProfileUrl.ts (1)
74-99: LGTM: Consistent handling ofsupportedflag.All return paths correctly include
supported: truewhen a platform is matched. The implementation is consistent, though the utility of the field depends on addressing the earlier comments about unsupported platforms.controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts (3)
8-21: LGTM: Proper handler signature and input validation.The handler correctly uses
RequestHandlertype and validates the requiredartist_account_idparameter with appropriate error handling.
23-36: LGTM: Proper error handling for fetching socials.The handler correctly handles both error states and empty results from
getAccountSocials.
63-70: LGTM: Appropriate error handling.The catch block properly logs errors and returns a generic 500 response without exposing internal details.
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts (1)
35-42: Include social identifiers in scrape results to enable result mapping.The batch results lack identifiers to map each
runId/datasetId/errorback to its corresponding social account. WhilePromise.allpreserves order, relying on positional mapping is fragile—clients cannot correlate results to socials without explicit identifiers likesocial_id,username, orprofile_url.Consider enriching the batch results with identifiers after scraping:
const results = await scrapeProfileUrlBatch( socials.map((social) => ({ profileUrl: social.profile_url, username: social.username, })) ); - res.json(results); + const enrichedResults = results.map((result, index) => ({ + social_id: socials[index].id, + username: socials[index].username, + profile_url: socials[index].profile_url, + ...result, + })); + + res.json(enrichedResults);Alternatively, extend
scrapeProfileUrlBatchto accept and preserve arbitrary metadata (e.g., a genericcontextfield) so identifiers can be passed through the batch operation.
🧹 Nitpick comments (1)
lib/apify/scrapeProfileUrlBatch.ts (1)
21-27: Consider preserving thesupportedfield in batch results.The function filters for
ScrapeProfileResult(which includes asupportedboolean indicating platform support), but the mapping drops this field and returnsProfileScrapeResult[]. Clients lose visibility into whether each profile URL was supported by a platform scraper or simply returned null for other reasons.If the
supportedfield is useful to consumers, consider updating the return type and mapping:export const scrapeProfileUrlBatch = async ( inputs: ScrapeProfileUrlBatchInput[] -): Promise<ProfileScrapeResult[]> => { +): Promise<ScrapeProfileResult[]> => { const results = await Promise.all( inputs.map(({ profileUrl, username }) => scrapeProfileUrl(profileUrl ?? null, username ?? "") ) ); return results - .filter((result): result is ScrapeProfileResult => result !== null) - .map(({ runId, datasetId, error }) => ({ - runId, - datasetId, - error, - })); + .filter((result): result is ScrapeProfileResult => result !== null); };Alternatively, if the
supportedfield is intentionally omitted for batch results, document this design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts(1 hunks)lib/apify/scrapeProfileUrlBatch.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/apify/scrapeProfileUrlBatch.ts (1)
lib/apify/scrapeProfileUrl.ts (3)
ProfileScrapeResult(15-19)scrapeProfileUrl(57-100)ScrapeProfileResult(21-23)
controllers/ArtistSocialsController/postArtistSocialsScrapeHandler.ts (3)
controllers/ArtistSocialsController/index.ts (1)
postArtistSocialsScrapeHandler(2-2)lib/supabase/getAccountSocials.ts (1)
getAccountSocials(9-64)lib/apify/scrapeProfileUrlBatch.ts (1)
scrapeProfileUrlBatch(12-28)
🔇 Additional comments (1)
lib/apify/scrapeProfileUrlBatch.ts (1)
17-17: Empty string defaults are validated downstream, but upstream filtering may be better.The platform scrapers (Instagram, TikTok) validate the handle parameter and throw "Invalid {Platform} handle" errors when empty strings are passed (after trimming). These errors are caught and returned in the result's error field, so failures are not silent—they're explicitly reported.
However, this still results in failed scrape attempts for any input with a missing username. Consider filtering out inputs with missing usernames upstream in
scrapeProfileUrlBatch.tsrather than defaulting to empty strings, which would prevent unnecessary API calls and error handling.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes