Conversation
|
Warning Rate limit exceeded@Blaumaus has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 58 seconds before requesting another review. ⌛ 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 (17)
WalkthroughAdds country-level blacklist support: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant AnalyticsService
participant GeoIP as Geo Service
participant Database
Client->>Controller: POST /analytics (payload, origin, IP)
Controller->>AnalyticsService: validate(logDTO, origin, ip)
AnalyticsService->>Database: fetch Project by origin
Database-->>AnalyticsService: Project
AnalyticsService-->>Controller: Project
Controller->>GeoIP: resolveCountry(ip)
GeoIP-->>Controller: countryCode or null
Controller->>AnalyticsService: checkCountryBlacklist(project, countryCode)
alt country is blacklisted
AnalyticsService-->>Controller: throws BadRequestException
Controller-->>Client: 400 Bad Request
else allowed
Controller->>Controller: collect device/browser/region info
Controller->>Database: insert analytics record
Database-->>Controller: OK
Controller-->>Client: 201 Created
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/apps/community/src/analytics/analytics.controller.ts (1)
885-910: Country blacklist is enforced after creating/updating sessionsIn
logCustom,log,noscript, andlogErroryou callgenerateAndStoreSessionIdand/orprocessInteractionSDbeforecheckCountryBlacklist(project, country). This means requests from blacklisted countries won’t write analytics rows, but they will still create/refresh sessions and contribute to heartbeat/“online visitors” style metrics.Consider moving
getGeoDetails(...)andcheckCountryBlacklist(...)earlier (right aftervalidate(...)and before any session or heartbeat side effects) so that blocked countries are excluded consistently across all derived stats.Also applies to: 1007-1030, 1141-1155, 1441-1456
backend/apps/cloud/src/analytics/analytics.controller.ts (1)
1151-1164: Country blacklist check runs after session creation, causing partial tracking of blocked usersIn
logError,logCustom,log, andnoscriptyou now:
- Call
validate(...)to getproject.- Create/refresh sessions via
generateAndStoreSessionIdandprocessInteractionSD.- Only then call
checkCountryBlacklist(project, country).For requests from blacklisted countries this prevents ClickHouse writes, but sessions and session duration keys are still created/updated, so:
- “Live visitors” / heartbeat counts can include users from blocked countries.
- Session duration stats may include interactions from those users.
Consider reordering to:
- Derive
countryviagetGeoDetails(...).- Run
checkCountryBlacklist(project, country).- Only if it passes, proceed with
generateAndStoreSessionId,processInteractionSD, and downstream inserts.This keeps country blocking semantics consistent across all analytics-derived metrics.
Also applies to: 1254-1276, 1377-1397, 1512-1525
backend/apps/cloud/src/analytics/analytics.service.ts (1)
462-503:validateHeartbeatcallers should capture the returnedProjectfor consistency and potential downstream checksAll
validate()call sites are correctly updated to capture theProjectreturn value (e.g.,const project = await this.analyticsService.validate(...)), and the returned object is used downstream. However, bothvalidateHeartbeat()call sites ignore the return value with bareawaitstatements, despite the method now returningPromise<Project>. This is inconsistent with thevalidate()pattern and may miss opportunities for the same downstream validation checks (like country blacklisting) thatvalidate()enables. Consider capturing the return value:const project = await this.analyticsService.validateHeartbeat(...)in both controllers.
🧹 Nitpick comments (6)
backend/apps/community/src/project/entity/project.entity.ts (1)
11-11: Consider nullable type for consistency with cloud entity and frontend.The cloud entity declares
countryBlacklistwith{ nullable: true, default: null }, and the frontend usesstring[] | null. This community model usesstring[]which doesn't account fornullvalues, potentially causing runtime issues when the field is null.- countryBlacklist: string[] + countryBlacklist: string[] | nullbackend/apps/cloud/src/analytics/analytics.service.ts (1)
433-451: Normalize blacklist entries to avoid case-sensitivity surprises
checkCountryBlacklistuppercases thecountryargument but not the entries inproject.countryBlacklist. If any codes are ever stored in lowercase/mixed case (e.g. via future APIs or migrations), they would silently bypass the check.You can make this more robust by normalizing the list as well:
- const countryBlacklist = _filter( - project.countryBlacklist, - Boolean, - ) as string[] + const countryBlacklist = _filter( + project.countryBlacklist, + Boolean, + ).map(cc => _toUpper(cc as string)) as string[]The rest of the logic can then continue to use
_includes(countryBlacklist, _toUpper(country)).web/app/pages/Project/Settings/tabs/Shields.tsx (1)
1-90: Country blacklist UI wiring looks solid; consider minor state-sync tweaksThe new country blacklist UI is well integrated: codes are sourced from
isoCountries, sorted perlanguage, and wired intoMultiSelectwith search and toggle handlers. A couple of small refinements to consider:
- Keep
searchedCountriesin sync withallCountrieswhen the language changes, so the visible list updates automatically:- const [searchedCountries, setSearchedCountries] = useState<string[]>(allCountries) + const [searchedCountries, setSearchedCountries] = useState<string[]>(allCountries) + + useEffect(() => { + setSearchedCountries(allCountries) + }, [allCountries])
- To be extra defensive, you could also normalize codes to uppercase in
setCountryBlacklist(though you already get uppercased alpha‑2 codes fromisoCountries).Overall, the UX and data flow here look good.
Also applies to: 113-131
web/app/pages/Project/Settings/ProjectSettings.tsx (1)
269-273: Country blacklist state wiring is correct; possible simplificationThe
countryBlacklistflow is coherent end‑to‑end:
- Form state always holds a
string[]([]by default), populated fromresult.countryBlacklist || [].onSubmitcorrectly serializes tonullwhen empty, matching typical backend patterns for optional arrays.- Shields receives
countryBlacklist={form.countryBlacklist || []}and a setter that updatesform.countryBlacklist, so edits propagate back into the main form.If you want to simplify later, you could drop the separate
countryBlacklistprop and letShieldsread/writeform.countryBlacklistdirectly (via asetFormcallback), but what you have now is perfectly valid.Also applies to: 286-296, 419-423, 465-485, 786-806
backend/apps/cloud/src/project/project.controller.ts (1)
1763-1770: Country blacklist mapping is consistent; consider adding validation / max-size guardThis block correctly mirrors the existing
ipBlacklisthandling and keepscountryBlacklisteitherstring[]ornull, which fits the new entity field. The only thing missing is any bound on how many country codes a client can send, which is also what CodeQL is complaining about here (loop over user-controlled list). To harden this and quiet the scanner, consider enforcing a reasonable maximum and element-shape at the DTO/validation layer (e.g. array-of-ISO codes with a small upper bound) rather than in the controller.backend/apps/cloud/src/project/dto/project.dto.ts (1)
46-52: ClarifycountryBlacklistshape in docs and optionally add validationThe new
countryBlacklistfield and its description look good, but two small improvements would help: (1) make the Swagger example match the actual JSON shape (["UA","RU","BY"]rather than a comma-separated string) to avoid ambiguity, and (2) optionally add class-validator decorators so this is treated as an optional array of ISO country codes with a sane max size, similar in spirit to how you intendipBlacklistto behave.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/apps/cloud/src/analytics/analytics.controller.ts(8 hunks)backend/apps/cloud/src/analytics/analytics.service.ts(4 hunks)backend/apps/cloud/src/project/dto/project.dto.ts(1 hunks)backend/apps/cloud/src/project/entity/project.entity.ts(1 hunks)backend/apps/cloud/src/project/project.controller.ts(1 hunks)backend/apps/community/src/analytics/analytics.controller.ts(8 hunks)backend/apps/community/src/analytics/analytics.service.ts(3 hunks)backend/apps/community/src/project/entity/project.entity.ts(1 hunks)web/app/lib/models/Project.ts(1 hunks)web/app/pages/Project/Settings/ProjectSettings.tsx(5 hunks)web/app/pages/Project/Settings/tabs/Shields.tsx(3 hunks)web/public/locales/en.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/community/src/analytics/analytics.controller.ts (1)
backend/apps/cloud/src/common/utils.ts (1)
getGeoDetails(254-283)
🪛 GitHub Check: CodeQL
backend/apps/cloud/src/project/project.controller.ts
[failure] 1765-1765: Loop bound injection
Iteration over a user-controlled object with a potentially unbounded .length property from a user-provided value.
🔇 Additional comments (5)
web/app/lib/models/Project.ts (1)
136-136: LGTM!The
countryBlacklistfield is correctly typed asstring[] | nullto match the backend entity, and is logically placed alongside the existingipBlacklistfield.web/public/locales/en.json (1)
1250-1253: LGTM!The locale strings are clear, follow existing naming conventions, and provide appropriate labels and hints for the new country blacklist feature.
backend/apps/community/src/analytics/analytics.service.ts (3)
398-416: LGTM!The implementation correctly mirrors the existing
checkIpBlacklistpattern with appropriate null handling, case-insensitive country code matching via_toUpper, and filtering of empty values. The early return for null country prevents unnecessary processing.
418-461: LGTM!Returning the validated
Projectinstead of void is a good design improvement. It enables callers to perform additional project-level checks (like country blacklist) without redundant Redis lookups, and maintains backward compatibility.
463-487: LGTM!The return type change is consistent with the
validatemethod modification, enabling country blacklist checks for heartbeat requests without redundant project lookups.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/migrations/clickhouse/initialise_selfhosted.js (1)
21-35: NewcountryBlacklistcolumn shape looks consistent with existing schemaDefining
countryBlacklistasNullable(String)alongsideipBlacklist Nullable(String)keeps the ClickHouse schema internally consistent and closer to the MySQLTEXT DEFAULT NULLcolumn. Just make sure the dedicated ClickHouse ALTER migration uses the sameNullable(String)type so freshly initialised DBs and migrated DBs have identical schemas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/apps/cloud/src/project/dto/project.dto.ts(1 hunks)backend/apps/community/src/common/utils.ts(2 hunks)backend/apps/community/src/project/dto/project.dto.ts(1 hunks)backend/apps/community/src/project/project.controller.ts(1 hunks)backend/migrations/clickhouse/initialise_selfhosted.js(1 hunks)backend/migrations/clickhouse/selfhosted_2025_11_30.js(1 hunks)backend/migrations/mysql/2025_11_30.sql(0 hunks)backend/migrations/mysql/2025_11_30_country_blacklist.sql(1 hunks)
💤 Files with no reviewable changes (1)
- backend/migrations/mysql/2025_11_30.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/cloud/src/project/dto/project.dto.ts
🧰 Additional context used
🧬 Code graph analysis (1)
backend/migrations/clickhouse/selfhosted_2025_11_30.js (1)
backend/migrations/clickhouse/setup.js (2)
dbName(107-107)queriesRunner(76-105)
🪛 GitHub Check: CodeQL
backend/apps/community/src/project/project.controller.ts
[failure] 947-947: Loop bound injection
Iteration over a user-controlled object with a potentially unbounded .length property from a user-provided value.
🔇 Additional comments (5)
backend/apps/community/src/common/utils.ts (2)
114-124: AddingcountryBlacklisttoALLOWED_KEYSis consistent with existing update flowIncluding
countryBlacklistalongsideipBlacklistensures project updates can safely propagate this new field viaupdateProjectClickhouse, reusing the existing mass-assignment guard.Please just double‑check that:
- The
project.countryBlacklistproperty exists on theProjectentity/DTOs in the community app, and- The ClickHouse
projecttable definescountryBlacklistas aString(or compatible) column,so that the
Stringbinding used inupdateProjectClickhousematches the schema.
441-454: DefaultcountryBlacklistinitialization matches existingipBlacklistbehaviorSetting
countryBlacklist: ''for new projects keeps defaults consistent withipBlacklistand avoids null-handling edge cases in downstream logic.To keep behavior uniform for existing data, ensure the ClickHouse migration:
- Adds
countryBlacklistwith a default of'', and- (If needed) backfills any pre‑existing rows to
''rather thanNULL.That will align stored values with what this insert path produces.
backend/migrations/mysql/2025_11_30_country_blacklist.sql (1)
1-1: MySQLcountryBlacklistmigration looks correct and consistentAdding
countryBlacklist TEXT DEFAULT NULL AFTER ipBlacklistmatches the existing blacklist pattern and nullable semantics. No issues from a schema standpoint.backend/apps/community/src/project/dto/project.dto.ts (1)
46-52: DTOcountryBlacklistproperty is well-documented and consistentThe new
countryBlacklist: string[] | nullfield, with the Swagger metadata you added, cleanly mirrorsipBlacklist/originsand clearly explains expected ISO 3166‑1 alpha‑2 values and behavior. Looks good.backend/apps/community/src/project/project.controller.ts (1)
945-952: Update logic forcountryBlacklistmatches existing patternsThe new block mirrors
origins/ipBlacklisthandling: trimming each entry and defaulting to an empty array when the field isn’t provided. That’s consistent with current update semantics and should integrate cleanly with the new DB column and DTO.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/app/ui/MultiSelect.tsx (2)
17-17: Fix the typo in the prop name.The prop is misspelled as
placholderinstead ofplaceholderin the interface definition (line 17), destructuring (line 31), and usage (line 95). This affects the component's public API.Apply this diff to fix the typo:
interface MultiSelectProps { className?: string onRemove: (item: any) => void onSelect: (item: any) => void items: any[] labelExtractor?: (item: any) => string keyExtractor?: (item: any) => string label: any[] hint?: string - placholder?: string + placeholder?: string searchPlaseholder?: string onSearch?: (search: string) => void itemExtractor?: (item: any) => string } const MultiSelect = ({ onRemove, onSelect, items, labelExtractor, keyExtractor, label, hint, - placholder = 'Select...', + placeholder = 'Select...', className, itemExtractor, onSearch, }: MultiSelectProps) => { // ... <input ref={inputRef} type='text' className='w-full cursor-text rounded-md border border-gray-300 bg-white py-2 pr-8 pl-3 text-sm transition-colors placeholder:text-gray-400 focus:border-indigo-500 focus:ring-1 focus:ring-indigo-500 focus:outline-hidden dark:border-gray-700 dark:bg-slate-800 dark:text-gray-50 dark:placeholder:text-gray-500' - placeholder={placholder} + placeholder={placeholder} value={searchValue} onChange={handleInputChange} onFocus={handleFocus} onKeyDown={handleKeyDown} />Also applies to: 31-31, 95-95
18-18: Remove the unused prop.The
searchPlaseholderprop is defined in the interface but never destructured or used in the component. This is dead code left over from the refactor.Apply this diff:
interface MultiSelectProps { className?: string onRemove: (item: any) => void onSelect: (item: any) => void items: any[] labelExtractor?: (item: any) => string keyExtractor?: (item: any) => string label: any[] hint?: string placholder?: string - searchPlaseholder?: string onSearch?: (search: string) => void itemExtractor?: (item: any) => string }
🧹 Nitpick comments (2)
web/app/ui/MultiSelect.tsx (2)
41-54: Consider memoizing theonSearchcallback in parent components.The
useEffectincludesonSearchin its dependency array. If the parent component doesn't memoize this callback (e.g., withuseCallback), the effect will re-run on every parent render, repeatedly adding and removing the event listener. While not critical, this could impact performance in frequently re-rendering parent components.
91-109: Consider adding accessibility attributes.The search input and dropdown could benefit from ARIA attributes to improve screen reader support:
- Add
aria-labeloraria-labelledbyto the input- Add
aria-expanded={isOpen}to indicate dropdown state- Add
role="combobox"to the input- Add
aria-controlsto link input to the dropdown listExample:
<input ref={inputRef} type='text' + role='combobox' + aria-expanded={isOpen} + aria-controls='multiselect-listbox' + aria-label='Search options' className='w-full cursor-text rounded-md border border-gray-300 bg-white py-2 pr-8 pl-3 text-sm transition-colors placeholder:text-gray-400 focus:border-indigo-500 focus:ring-1 focus:ring-indigo-500 focus:outline-hidden dark:border-gray-700 dark:bg-slate-800 dark:text-gray-50 dark:placeholder:text-gray-500' placeholder={placholder} value={searchValue} onChange={handleInputChange} onFocus={handleFocus} onKeyDown={handleKeyDown} />Additionally, the dropdown list should have
role="listbox"andid="multiselect-listbox", and each option should haverole="option".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/app/ui/MultiSelect.tsx(1 hunks)
🔇 Additional comments (3)
web/app/ui/MultiSelect.tsx (3)
36-85: LGTM! State management and event handlers are well-implemented.The state management (isOpen, searchValue) and all event handlers (input change, focus, keydown, select) are correctly implemented. The flow is logical:
- Opening dropdown on focus provides good UX
- Escape key properly closes and resets state
- Search value is propagated to parent via
onSearch- Item selection correctly invokes
onSelectand resets search
111-159: LGTM! Dropdown rendering is correct.The dropdown logic properly handles:
- Conditional rendering based on
isOpenstate- Empty state with a user-friendly message
- Item mapping with correct key extraction
- Selection indicators (checkmarks) for selected items
- Proper event handling via
handleSelectItem- Appropriate styling for selected vs unselected states
163-188: LGTM! Selected items display is well-implemented.The selected items chip display correctly:
- Shows only when items are selected (conditional on
!_isEmpty(label))- Uses proper key extraction for each chip
- Displays items using
labelExtractor- Remove button properly prevents event propagation
- Includes screen reader text for accessibility
- Styling supports both light and dark modes
1 participant
#424
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.