Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactors and micro-optimizations across helpers and services: precomputes localized creator labels, caches creator birthday info, centralizes movie-to-DTO mapping, and threads film type parameters into user-ratings and user-reviews builder methods (call sites updated accordingly). Changes
Sequence Diagram(s)(none) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/services/search.service.ts (1)
63-74: Consider applying the same mapper pattern to theusersblock for consistency.Movies and TV series now use
.map()+ spread, butusersstill usesforEach+push. AuserMapper+users.push(...usersNode.map(userMapper))would keep the style uniform. Low priority — purely a style nit.Optional refactor
- usersNode.forEach((m) => { - const url = getUserUrl(m); - users.push({ - id: parseIdFromUrl(url), - user: getUser(m), - userRealName: getUserRealName(m), - avatar: getAvatar(m), - url: `${baseUrl}${url}` - }); - }); + const userMapper = (m: HTMLElement): CSFDSearchUser => { + const url = getUserUrl(m); + return { + id: parseIdFromUrl(url), + user: getUser(m), + userRealName: getUserRealName(m), + avatar: getAvatar(m), + url: `${baseUrl}${url}` + }; + }; + + users.push(...usersNode.map(userMapper));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/search.service.ts` around lines 63 - 74, Replace the users forEach block with a mapper function to match the tvSeries pattern: create a userMapper that accepts a node (same shape as used in usersNode) and returns an object with id: parseIdFromUrl(getUserUrl(m)), user: getUser(m), userRealName: getUserRealName(m), avatar: getAvatar(m), url: `${baseUrl}${getUserUrl(m)}`, then replace the forEach+push loop by users.push(...usersNode.map(userMapper)) so usersNode, getUserUrl, parseIdFromUrl, getUser, getUserRealName, getAvatar, baseUrl and the users array are used consistently.src/helpers/movie.helper.ts (1)
344-348:Array.fromis redundant here.
node-html-parser'squerySelectorAllalready returnsHTMLElement[]. TheArray.fromwrap is harmless but unnecessary.Also, the return type is declared as
HTMLElement, but the function can returnundefined(whenfindhas no match,?.parentNodeyieldsundefined). Theas HTMLElementcast silently converts this. Callers do guard with?., so it works in practice, but a more honest signature would beHTMLElement | undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/movie.helper.ts` around lines 344 - 348, The helper getBoxContent unnecessarily wraps el.querySelectorAll in Array.from and unsafely casts the possibly-undefined result to HTMLElement; remove the redundant Array.from and change the function signature to return HTMLElement | undefined, and stop force-casting the result—let headers be the querySelectorAll result and return headers.find(... )?.parentNode (typed as possibly undefined) so callers can continue to guard with optional chaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/helpers/movie.helper.ts`:
- Around line 344-348: The helper getBoxContent unnecessarily wraps
el.querySelectorAll in Array.from and unsafely casts the possibly-undefined
result to HTMLElement; remove the redundant Array.from and change the function
signature to return HTMLElement | undefined, and stop force-casting the
result—let headers be the querySelectorAll result and return headers.find(...
)?.parentNode (typed as possibly undefined) so callers can continue to guard
with optional chaining.
In `@src/services/search.service.ts`:
- Around line 63-74: Replace the users forEach block with a mapper function to
match the tvSeries pattern: create a userMapper that accepts a node (same shape
as used in usersNode) and returns an object with id:
parseIdFromUrl(getUserUrl(m)), user: getUser(m), userRealName:
getUserRealName(m), avatar: getAvatar(m), url: `${baseUrl}${getUserUrl(m)}`,
then replace the forEach+push loop by users.push(...usersNode.map(userMapper))
so usersNode, getUserUrl, parseIdFromUrl, getUser, getUserRealName, getAvatar,
baseUrl and the users array are used consistently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/helpers/movie.helper.tssrc/services/creator.service.tssrc/services/search.service.tssrc/services/user-ratings.service.tssrc/services/user-reviews.service.ts
bartholomej
force-pushed
the
perf-improv
branch
2 times, most recently
from
694e415 to
2732b0d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/helpers/movie.helper.ts (1)
304-317: Solid optimization; consider addingbreakto short-circuit the inner scan.The precomputation of
localizedLabelscorrectly reducesgetLocalizedCreatorLabelcalls from O(groups × keys) to O(keys) per invocation — good improvement.Two minor follow-ups worth considering:
- The inner
for...ofoverlocalizedLabelshas nobreakafter a match, so all 10 labels are still checked even once a group is matched. Since each DOM<h4>corresponds to at most one creator category (for a fixed language), abreakis safe and makes the intent clearer.getLocalizedCreatorLabelis still called 10 times pergetMovieCreatorsinvocation (once per key), each time allocating the full three-languagelabelsdictionary. Computing the language-specific sub-record once before thekeys.map(...)could eliminate those repeated allocations entirely.♻️ Optional: early-exit on match + single-pass label resolution
+ const lang = options?.language || 'cs'; + // Resolve once: pick the language sub-record, fall back to Czech + // (mirrors the existing fallback logic inside getLocalizedCreatorLabel) + // If you prefer to keep the helper encapsulated, just add the break below. + const localizedLabels = keys.map((key) => ({ key, - label: getLocalizedCreatorLabel(options?.language, key) as string + label: getLocalizedCreatorLabel(lang, key) as string })); for (const group of groups) { const text = group.textContent.trim(); for (const { key, label } of localizedLabels) { if (text.includes(label)) { if (group.parentNode) { creators[key] = parseMoviePeople(group.parentNode as HTMLElement); } + break; // each h4 maps to at most one creator category } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/movie.helper.ts` around lines 304 - 317, The inner scan over localizedLabels should short-circuit once a match is found and also avoid repeated allocations from getLocalizedCreatorLabel: compute the language-specific label map once (call getLocalizedCreatorLabel or equivalent to build a per-language dictionary before keys.map) and use that in localizedLabels, then inside the groups loop, after assigning creators[key] = parseMoviePeople(group.parentNode as HTMLElement) add a break to stop checking other labels for that group; update references to localizedLabels, keys.map, getLocalizedCreatorLabel, groups, creators, and parseMoviePeople accordingly.src/services/search.service.ts (1)
43-74: Clean deduplication viamovieMapper;usersNodeblock is slightly inconsistent.The
movieMapperclosure is a well-placed refactor — it eliminates the duplicated inline object construction for movies and TV series, andbaseUrlcaptured in the closure is aconstso there's no stale-reference risk.The
usersNode.forEachblock (lines 63–72) is the only remaining call site that still uses thepush-inside-forEachpattern rather than the newpush(...node.map(mapper))style. Not a bug, but worth aligning for consistency in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/search.service.ts` around lines 43 - 74, The usersNode.forEach block is inconsistent with the refactor that introduced movieMapper; create a userMapper (or reuse a closure) that builds the user object (using getUserUrl, parseIdFromUrl, getUser, getUserRealName, getAvatar and baseUrl) and replace the forEach push pattern with users.push(...usersNode.map(userMapper)) to match the movies/tvSeries style and remove the push-inside-forEach anti-pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/helpers/movie.helper.ts`:
- Around line 304-317: The inner scan over localizedLabels should short-circuit
once a match is found and also avoid repeated allocations from
getLocalizedCreatorLabel: compute the language-specific label map once (call
getLocalizedCreatorLabel or equivalent to build a per-language dictionary before
keys.map) and use that in localizedLabels, then inside the groups loop, after
assigning creators[key] = parseMoviePeople(group.parentNode as HTMLElement) add
a break to stop checking other labels for that group; update references to
localizedLabels, keys.map, getLocalizedCreatorLabel, groups, creators, and
parseMoviePeople accordingly.
In `@src/services/search.service.ts`:
- Around line 43-74: The usersNode.forEach block is inconsistent with the
refactor that introduced movieMapper; create a userMapper (or reuse a closure)
that builds the user object (using getUserUrl, parseIdFromUrl, getUser,
getUserRealName, getAvatar and baseUrl) and replace the forEach push pattern
with users.push(...usersNode.map(userMapper)) to match the movies/tvSeries style
and remove the push-inside-forEach anti-pattern.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/helpers/movie.helper.tssrc/services/creator.service.tssrc/services/search.service.tssrc/services/user-ratings.service.tssrc/services/user-reviews.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/creator.service.ts
bartholomej
force-pushed
the
perf-improv
branch
from
2732b0d to
32c2265
Compare
1 participant
Description
Performance improvements
Type of change
Related Issues
Checklist
Summary by CodeRabbit