react native support#997
Open
goldflag wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces mobile site type support to the analytics platform alongside a complete React Native SDK. It adds a ChangesMobile site type and React Native SDK support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/components/SiteSettings/GeneralTab.tsx (1)
121-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate mobile identifiers before submit and return mobile-specific failure text.
In Line 129, mobile updates are only trimmed and sent; malformed values are deferred to server rejection, and Line 136 still reports a domain-specific failure for mobile sites.
Suggested fix
const handleDomainChange = async () => { - if (!newDomain) { + const trimmedValue = newDomain.trim(); + if (!trimmedValue) { toast.error(isMobileSite ? t("App identifier cannot be empty") : t("Domain cannot be empty")); return; } + if (isMobileSite && !/^(?!.*\.\.)(?!\.)(?!.*\.$)[A-Za-z][A-Za-z0-9_-]*(\.[A-Za-z][A-Za-z0-9_-]*)+$/.test(trimmedValue)) { + toast.error(t("Invalid app identifier. Use a bundle/package identifier like com.example.app")); + return; + } try { setIsChangingDomain(true); - const normalizedDomain = isMobileSite ? newDomain.trim() : normalizeDomain(newDomain); + const normalizedDomain = isMobileSite ? trimmedValue : normalizeDomain(trimmedValue); await updateSiteConfig(siteMetadata.siteId, { domain: normalizedDomain }); toast.success(isMobileSite ? t("App identifier updated successfully") : t("Domain updated successfully")); router.refresh(); refetch(); } catch (error) { console.error("Error changing domain:", error); - toast.error(t("Failed to update domain")); + toast.error(isMobileSite ? t("Failed to update app identifier") : t("Failed to update domain")); } finally { setIsChangingDomain(false); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/SiteSettings/GeneralTab.tsx` around lines 121 - 137, The handleDomainChange currently only trims mobile identifiers and still uses domain-specific error messages; update it to perform client-side validation for mobile sites: after computing normalizedDomain (from newDomain.trim() when isMobileSite), validate against the mobile identifier rules (non-empty, allowed characters/format) and if invalid call toast.error with the mobile-specific message (e.g., t("Invalid app identifier")) and return; also in the catch block use the mobile-specific failure text when isMobileSite (instead of the domain message) so updateSiteConfig failures show the correct mobile error; refer to handleDomainChange, isMobileSite, newDomain, normalizeDomain, updateSiteConfig, and the toast.* calls to locate where to add the validation and conditional error messaging.server/src/services/tracker/identifyService.ts (1)
101-127:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCollapse the alias write into a single upsert.
This is still a select-then-write race. Two concurrent identify calls for the same
anonymousIdcan both miss the row; one insert wins and the other falls into the catch block, leaving the alias on the wronguser_idand skipping the backfill path. A singleonConflictDoUpdate(...)write avoids that nondeterminism.Suggested direction
- // Check if alias already exists - const existingAlias = await db - .select() - .from(userAliases) - .where(and(eq(userAliases.siteId, siteId), eq(userAliases.anonymousId, anonymousId))) - .limit(1); - - if (existingAlias.length === 0) { - // Create new alias - await db.insert(userAliases).values({ - siteId, - anonymousId, - userId: user_id, - }); - // Fire-and-forget: backfill identified_user_id on past anonymous events - backfillIdentifiedUserId(siteId, anonymousId, user_id); - } else if (existingAlias[0].userId !== user_id) { - // Update alias to point to new user - await db - .update(userAliases) - .set({ userId: user_id }) - .where(and(eq(userAliases.siteId, siteId), eq(userAliases.anonymousId, anonymousId))); - } + await db + .insert(userAliases) + .values({ + siteId, + anonymousId, + userId: user_id, + }) + .onConflictDoUpdate({ + target: [userAliases.siteId, userAliases.anonymousId], + set: { userId: user_id }, + }); + + // Trigger backfill according to the intended insert/update semantics. + backfillIdentifiedUserId(siteId, anonymousId, user_id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/services/tracker/identifyService.ts` around lines 101 - 127, The current select-then-insert/update in the identifyService.ts around userAliases is racy: replace the read-then-write with a single upsert using db.insert(...).onConflictDoUpdate(...) (targeting userAliases.siteId + userAliases.anonymousId) so concurrent identify calls deterministically set userId; ensure the upsert's update clause sets userId to the incoming user_id and returns whether the row was inserted vs updated (or check returned rows) so you can call backfillIdentifiedUserId(siteId, anonymousId, user_id) only when the alias was newly created (or when userId actually changed), removing the select and catching unique-constraint races in the try/catch around this block.
🧹 Nitpick comments (1)
react-native/index.d.ts (1)
59-72: 💤 Low valueConsider making
getUserId()async for API consistency.The
getUserId()method returnsstring | nullsynchronously, while all other SDK methods are async and returnPromise<void>orPromise<T>. This inconsistency may confuse developers expecting a uniform async API throughout the SDK.If the user ID is always available in memory (no async storage read needed), document this explicitly. Otherwise, consider making it async:
getUserId(): Promise<string | null>to match the rest of the API surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@react-native/index.d.ts` around lines 59 - 72, The getUserId method on class RybbitReactNative is synchronous while the rest of the SDK surface is async; either document that userId is always held in memory or change the signature to async for consistency—update the declaration in RybbitReactNative from getUserId(): string | null to getUserId(): Promise<string | null>, update any implementations of getUserId to return a Promise (resolve the in-memory value or await storage), and adjust any callers to await RybbitReactNative.getUserId() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/messages/cs.json`:
- Line 68: The Czech translations file client/messages/cs.json contains multiple
keys with empty values (e.g., "erYzJH", "FIKvUB", "KI57sV", "AZEGID", "JkLHGw",
"CXI0EW", "VQxsRK", "W35dz6", "2YpSsF", "HtTwhf", "zoRFJV", "pm+3nT", "0Q1jqR",
"8s+sTd", "495/Ut", "lxHUeO") that must be completed; open cs.json, locate each
empty key (also present at the other mentioned occurrences) and replace the ""
with accurate Czech translations matching the feature/context (mobile site
support, app identifier validation, tracking UI labels), ensuring string
formatting and placeholders match the English source and run a quick
linter/locale check after updating.
In `@client/messages/de.json`:
- Line 68: Several German translation keys are empty which will render blank UI
text; locate the entries for keys erYzJH, FIKvUB, KI57sV, AZEGID, JkLHGw,
CXI0EW, VQxsRK, W35dz6, 2YpSsF, HtTwhf, zoRFJV, pm+3nT, 0Q1jqR, 8s+sTd, 495/Ut
and lxHUeO and replace the empty string values with appropriate German
translations (matching the intent of their English originals for mobile site
features, app ID validation, and tracking text); ensure formatting/escaping
matches the surrounding JSON, run the i18n/lint or build checks to validate
there are no missing keys or formatting errors, and include meaningful,
context-aware German phrases for each key.
In `@client/messages/es.json`:
- Line 68: Several i18n keys (e.g., "erYzJH" and the other keys listed at lines
87, 636, 640-642, 644, 650, 1105, 1107, 1128, 1183-1184, 1223, 1225, 1233) are
set to empty strings which yields blank UI text; for each such key either
provide the proper Spanish translation string or remove the key entirely from
client/messages/es.json until a translation is available so the i18n runtime can
fall back to the default locale—update the values for keys like "erYzJH" (and
the others referenced) with their Spanish text or delete those entries to avoid
blank labels.
In `@client/messages/fr.json`:
- Line 68: The JSON contains empty-string translations (e.g., key "erYzJH" and
the other listed keys at lines mentioned) which must not be shipped; either
provide proper French translations for each listed key (replace "" with the
translated string) or remove/omit the keys temporarily from
client/messages/fr.json until translations are available, then run the i18n
build/validation step to ensure no empty values remain; specifically update the
keys "erYzJH" and the other keys called out (lines 87, 636, 640-642, 644, 650,
1105, 1107, 1128, 1183-1184, 1223, 1225, 1233) so none have empty values.
In `@client/messages/it.json`:
- Line 68: Several Italian translation entries are empty (e.g., the key "erYzJH"
and also keys at positions corresponding to the other empty entries listed);
either provide proper Italian copy for each empty JSON key or remove the key
entirely so the app falls back to the default locale. Update the JSON keys
"erYzJH" and the other empty keys (the ones reported at 87, 636, 640-642, 644,
650, 1105, 1107, 1128, 1183-1184, 1223, 1225, 1233) with valid Italian strings
or delete those key entries to avoid rendering blank UI text.
In `@client/messages/ja.json`:
- Line 68: The Japanese messages file contains empty translation values (e.g.,
the key "erYzJH" and the other listed keys), which will render blank labels;
replace each empty string with the proper Japanese translation or temporarily
remove the key until a localized copy is available, ensuring you update the same
keys (e.g., "erYzJH") in client/messages/ja.json and keep JSON syntax valid;
after edits run the i18n/localization linter or build to confirm no missing
keys/formatting issues.
In `@client/messages/ko.json`:
- Line 68: Replace empty translation values in the Korean locale with at least
temporary fallback copy (e.g., the original English source text) so UI labels
don't render blank; specifically update the "erYzJH" entry to a non-empty string
and find and fix the other empty keys referenced in the comment (the entries at
the indicated positions such as those around lines 87, 636, 640-642, 644, 650,
1105, 1107, 1128, 1183-1184, 1223, 1225, 1233) in client/messages/ko.json by
populating each empty value with the English source or plausible placeholder
until proper Korean translations are available.
In `@client/messages/pl.json`:
- Line 68: Several newly added localization keys (e.g., "erYzJH" plus the other
keys called out at lines like 87, 636, 640-642, 644, 650, 1105, 1107, 1128,
1183-1184, 1223, 1225, 1233) are empty strings in client/messages/pl.json;
replace each empty value with an appropriate Polish translation or a temporary
Polish fallback copy (e.g., English fallback wrapped in Polish context) so the
UI won't render blank labels/messages—update the entries for keys "erYzJH" and
the other listed keys to non-empty Polish text while preserving the exact JSON
keys and formatting.
In `@client/messages/pt.json`:
- Line 68: The JSON translation entry "erYzJH" (and the other empty keys listed:
entries at 87, 636, 640-642, 644, 650, 1105, 1107, 1128, 1183-1184, 1223, 1225,
1233) are empty placeholders and must be populated; open
client/messages/pt.json, locate each empty key (e.g., "erYzJH") and provide the
correct Portuguese translation for the source string, or if unsure copy the
original English source text as a temporary fallback so the UI won't show blank
text. Ensure each replaced value is a valid JSON string (escaped as needed) and
keep the key names unchanged.
In `@client/messages/uk.json`:
- Line 68: Several message keys in the Ukrainian localization are empty and must
be populated; update the entries for keys "erYzJH" and the other affected keys
(those at positions you noted: the ones corresponding to lines 87, 636, 640-642,
644, 650, 1105, 1107, 1128, 1183-1184, 1223, 1225, 1233) so they are not empty
strings—either provide proper Ukrainian translations or insert a temporary
fallback (e.g., the English source text) for each key; ensure you edit the
uk.json entries for each listed key (leave JSON formatting intact) and run a
quick lint/validate of the JSON to confirm no syntax errors.
In `@client/messages/zh.json`:
- Line 68: Several Chinese locale keys were left as empty strings (e.g.,
"erYzJH" and the other keys at the comment: those at positions 87, 636, 640-642,
644, 650, 1105, 1107, 1128, 1183-1184, 1223, 1225, 1233) causing blank labels;
update each of these keys in the zh.json file to provide either the proper
Chinese translation or a temporary fallback English copy so the app doesn't
render empty text, ensuring you edit the exact JSON keys (like "erYzJH") and
keep valid JSON string values.
In `@client/src/app/components/AddSite.tsx`:
- Around line 32-33: The isValidAppIdentifier validator currently allows
single-segment names like "myapp" and permits a leading digit; update the
function (isValidAppIdentifier) to enforce a package/bundle style (dot-separated
segments, at least two segments) where each segment starts with a letter and
uses only letters, digits, underscores, hyphens or dots as separators, and
preserve overall length limits (e.g. max 253 chars total). Replace the current
regex with one that requires ^[A-Za-z][A-Za-z0-9_-]*(\.[A-Za-z][A-Za-z0-9_-]*)+$
so identifiers like "com.example.app" are valid but "myapp" or "1app" are
rejected; update any tests or usage that rely on the old behavior.
In `@react-native/index.js`:
- Line 3: The SDK_VERSION constant in react-native/index.js is hardcoded and can
drift from package.json; replace the literal SDK_VERSION with the package
version by importing the version field from package.json (e.g., require or
import the JSON and use its .version) and assign that to SDK_VERSION so it
always matches package.json; update any references to SDK_VERSION to use the new
imported value.
- Around line 204-227: createBasePayload currently falls back to
this.config.siteId for the hostname which can put numeric site IDs into the
hostname field; enforce a proper app identifier by (1) changing init() to
validate that this.config.appIdentifier (or context.appIdentifier) is present
for mobile/React Native usage and throw or log a clear error if missing, and (2)
updating createBasePayload to use only context.appIdentifier ||
this.config.appIdentifier for hostname (do not fallback to this.config.siteId);
reference createBasePayload and init (or the module initialization path) and
update any docs/tests that construct the client so mobile integrations supply a
bundle/app identifier like com.example.app.
In `@server/src/api/sites/addSite.ts`:
- Around line 70-80: The current appIdentifierRegex is too permissive and allows
single-token or trailing-dot values; update the regex used in the siteType ===
"mobile" validation (symbol: appIdentifierRegex and variable cleanedDomain in
addSite.ts) to enforce bundle/package format: require at least two dot-separated
components, no leading/trailing/consecutive dots, each component starting with a
letter and containing only letters, digits or underscores/hyphens as allowed
(e.g. replace with something like
^[A-Za-z][A-Za-z0-9_-]*(?:\.[A-Za-z][A-Za-z0-9_-]*)+$), then use that regex in
the existing if (siteType === "mobile" &&
!appIdentifierRegex.test(cleanedDomain)) branch and update the error message to
mention the required dot-separated bundle identifier format.
In `@server/src/api/sites/updateSiteConfig.ts`:
- Around line 87-88: The code uses nextDomain = updateData.domain ?? site.domain
and immediately calls replace() on it, which can throw when site.domain is
nullish; update the logic around nextDomain/cleanedDomain in updateSiteConfig.ts
to guard for null/undefined before calling replace(): compute nextDomain as
before, then if nextDomain is nullish skip normalization (e.g., leave
cleanedDomain undefined or null) or normalize via a safe fallback like
(nextDomain ?? "") before replace, and ensure the later domain/type validation
still rejects absent domains as intended; update references to nextDomain and
cleanedDomain accordingly so unrelated PATCHes don't hit replace() on nullish
site.domain.
In `@server/src/services/userId/userIdService.ts`:
- Around line 70-74: The generateUserIdFromClientId function is using
getDailySalt(), causing client-derived anonymous_id hashes to change each UTC
day; replace the daily-rotating salt with a stable site-specific salt so the
same clientId always hashes to the same user id across days. Update
generateUserIdFromClientId to use a persistent salt from the site config (e.g.,
a config.userIdSalt or config.salt) or an empty string when saltUserIds is false
instead of calling this.getDailySalt(), ensuring the hash input remains stable;
adjust any references to getDailySalt() here to a stable-salt accessor or field.
---
Outside diff comments:
In `@client/src/components/SiteSettings/GeneralTab.tsx`:
- Around line 121-137: The handleDomainChange currently only trims mobile
identifiers and still uses domain-specific error messages; update it to perform
client-side validation for mobile sites: after computing normalizedDomain (from
newDomain.trim() when isMobileSite), validate against the mobile identifier
rules (non-empty, allowed characters/format) and if invalid call toast.error
with the mobile-specific message (e.g., t("Invalid app identifier")) and return;
also in the catch block use the mobile-specific failure text when isMobileSite
(instead of the domain message) so updateSiteConfig failures show the correct
mobile error; refer to handleDomainChange, isMobileSite, newDomain,
normalizeDomain, updateSiteConfig, and the toast.* calls to locate where to add
the validation and conditional error messaging.
In `@server/src/services/tracker/identifyService.ts`:
- Around line 101-127: The current select-then-insert/update in the
identifyService.ts around userAliases is racy: replace the read-then-write with
a single upsert using db.insert(...).onConflictDoUpdate(...) (targeting
userAliases.siteId + userAliases.anonymousId) so concurrent identify calls
deterministically set userId; ensure the upsert's update clause sets userId to
the incoming user_id and returns whether the row was inserted vs updated (or
check returned rows) so you can call backfillIdentifiedUserId(siteId,
anonymousId, user_id) only when the alias was newly created (or when userId
actually changed), removing the select and catching unique-constraint races in
the try/catch around this block.
---
Nitpick comments:
In `@react-native/index.d.ts`:
- Around line 59-72: The getUserId method on class RybbitReactNative is
synchronous while the rest of the SDK surface is async; either document that
userId is always held in memory or change the signature to async for
consistency—update the declaration in RybbitReactNative from getUserId(): string
| null to getUserId(): Promise<string | null>, update any implementations of
getUserId to return a Promise (resolve the in-memory value or await storage),
and adjust any callers to await RybbitReactNative.getUserId() accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54ead776-c6e5-417b-8000-052113750926
📒 Files selected for processing (42)
client/messages/cs.jsonclient/messages/de.jsonclient/messages/en.jsonclient/messages/es.jsonclient/messages/fr.jsonclient/messages/it.jsonclient/messages/ja.jsonclient/messages/ko.jsonclient/messages/pl.jsonclient/messages/pt.jsonclient/messages/uk.jsonclient/messages/zh.jsonclient/src/api/admin/endpoints/adminSites.tsclient/src/api/admin/endpoints/sites.tsclient/src/app/[site]/components/Sidebar/Sidebar.tsxclient/src/app/[site]/components/Sidebar/SiteSelector.tsxclient/src/app/components/AddSite.tsxclient/src/components/SiteSettings/GeneralTab.tsxclient/src/components/SiteSettings/ScriptBuilder.tsxclient/src/components/SiteSettings/SiteSettings.tsxclient/src/components/SiteSettings/TrackingTab.tsxdocs/content/docs/sdks/meta.jsondocs/content/docs/sdks/react-native.mdxreact-native/README.mdreact-native/index.d.tsreact-native/index.jsreact-native/package.jsonserver/drizzle/0006_site_type.sqlserver/drizzle/meta/0006_snapshot.jsonserver/drizzle/meta/_journal.jsonserver/src/api/admin/getAdminSites.tsserver/src/api/sites/addSite.tsserver/src/api/sites/getSite.tsserver/src/api/sites/getSitesFromOrg.tsserver/src/api/sites/getTrackingConfig.tsserver/src/api/sites/updateSiteConfig.tsserver/src/db/postgres/schema.tsserver/src/lib/siteConfig.tsserver/src/services/tracker/identifyService.tsserver/src/services/tracker/trackEvent.tsserver/src/services/tracker/utils.tsserver/src/services/userId/userIdService.ts
| "tL4WC1": "Načítám další...", | ||
| "HPx76n": "Všechny položky načteny", | ||
| "QkTyho": "Postranní panel Rybbit", | ||
| "erYzJH": "", |
Contributor
There was a problem hiding this comment.
Complete Czech translations for mobile site features.
Multiple translation keys have been added with empty string values (""), which will display as blank text in the Czech UI. These appear to be for mobile site type support, app identifier validation, and tracking features.
Empty translation keys: erYzJH, FIKvUB, KI57sV, AZEGID, JkLHGw, CXI0EW, VQxsRK, W35dz6, 2YpSsF, HtTwhf, zoRFJV, pm+3nT, 0Q1jqR, 8s+sTd, 495/Ut, lxHUeO.
Consider completing these translations before the first stable release to ensure a complete user experience for Czech-speaking users.
Also applies to: 87-87, 636-636, 640-642, 644-644, 650-650, 1105-1105, 1107-1107, 1128-1128, 1183-1184, 1223-1223, 1225-1225, 1233-1233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/messages/cs.json` at line 68, The Czech translations file
client/messages/cs.json contains multiple keys with empty values (e.g.,
"erYzJH", "FIKvUB", "KI57sV", "AZEGID", "JkLHGw", "CXI0EW", "VQxsRK", "W35dz6",
"2YpSsF", "HtTwhf", "zoRFJV", "pm+3nT", "0Q1jqR", "8s+sTd", "495/Ut", "lxHUeO")
that must be completed; open cs.json, locate each empty key (also present at the
other mentioned occurrences) and replace the "" with accurate Czech translations
matching the feature/context (mobile site support, app identifier validation,
tracking UI labels), ensuring string formatting and placeholders match the
English source and run a quick linter/locale check after updating.
| "tL4WC1": "Lade mehr...", | ||
| "HPx76n": "Alle Einträge geladen", | ||
| "QkTyho": "Rybbit-Seitenleiste", | ||
| "erYzJH": "", |
Contributor
There was a problem hiding this comment.
Complete German translations for mobile site features.
Multiple translation keys have been added with empty string values (""), which will display as blank text in the German UI. These appear to be for mobile site type support, app identifier validation, and tracking features.
Empty translation keys: erYzJH, FIKvUB, KI57sV, AZEGID, JkLHGw, CXI0EW, VQxsRK, W35dz6, 2YpSsF, HtTwhf, zoRFJV, pm+3nT, 0Q1jqR, 8s+sTd, 495/Ut, lxHUeO.
Consider completing these translations before the first stable release to ensure a complete user experience for German-speaking users.
Also applies to: 87-87, 636-636, 640-642, 644-644, 650-650, 1105-1105, 1107-1107, 1128-1128, 1183-1184, 1223-1223, 1225-1225, 1233-1233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/messages/de.json` at line 68, Several German translation keys are
empty which will render blank UI text; locate the entries for keys erYzJH,
FIKvUB, KI57sV, AZEGID, JkLHGw, CXI0EW, VQxsRK, W35dz6, 2YpSsF, HtTwhf, zoRFJV,
pm+3nT, 0Q1jqR, 8s+sTd, 495/Ut and lxHUeO and replace the empty string values
with appropriate German translations (matching the intent of their English
originals for mobile site features, app ID validation, and tracking text);
ensure formatting/escaping matches the surrounding JSON, run the i18n/lint or
build checks to validate there are no missing keys or formatting errors, and
include meaningful, context-aware German phrases for each key.
| "tL4WC1": "Cargando más...", | ||
| "HPx76n": "Todos los elementos cargados", | ||
| "QkTyho": "Barra lateral de Rybbit", | ||
| "erYzJH": "", |
Contributor
There was a problem hiding this comment.
Replace empty translation values to avoid blank UI text.
These newly introduced keys are set to "". In most i18n runtimes that results in blank labels/messages instead of fallback text, which breaks parts of the Spanish UI flow. Please provide translations now or remove these keys until translations are ready so fallback can work.
Also applies to: 87-87, 636-636, 640-642, 644-644, 650-650, 1105-1105, 1107-1107, 1128-1128, 1183-1184, 1223-1223, 1225-1225, 1233-1233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/messages/es.json` at line 68, Several i18n keys (e.g., "erYzJH" and
the other keys listed at lines 87, 636, 640-642, 644, 650, 1105, 1107, 1128,
1183-1184, 1223, 1225, 1233) are set to empty strings which yields blank UI
text; for each such key either provide the proper Spanish translation string or
remove the key entirely from client/messages/es.json until a translation is
available so the i18n runtime can fall back to the default locale—update the
values for keys like "erYzJH" (and the others referenced) with their Spanish
text or delete those entries to avoid blank labels.
| "tL4WC1": "Chargement en cours...", | ||
| "HPx76n": "Tous les éléments chargés", | ||
| "QkTyho": "Barre latérale Rybbit", | ||
| "erYzJH": "", |
Contributor
There was a problem hiding this comment.
Do not ship empty-string translations in French locale.
The new keys on these lines are empty (""), which will surface as missing/blank UI copy and can suppress fallback behavior. Please translate them or temporarily omit these keys until translations are available.
Also applies to: 87-87, 636-636, 640-642, 644-644, 650-650, 1105-1105, 1107-1107, 1128-1128, 1183-1184, 1223-1223, 1225-1225, 1233-1233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/messages/fr.json` at line 68, The JSON contains empty-string
translations (e.g., key "erYzJH" and the other listed keys at lines mentioned)
which must not be shipped; either provide proper French translations for each
listed key (replace "" with the translated string) or remove/omit the keys
temporarily from client/messages/fr.json until translations are available, then
run the i18n build/validation step to ensure no empty values remain;
specifically update the keys "erYzJH" and the other keys called out (lines 87,
636, 640-642, 644, 650, 1105, 1107, 1128, 1183-1184, 1223, 1225, 1233) so none
have empty values.
| "tL4WC1": "Caricamento in corso...", | ||
| "HPx76n": "Tutti gli elementi caricati", | ||
| "QkTyho": "Barra Laterale Rybbit", | ||
| "erYzJH": "", |
Contributor
There was a problem hiding this comment.
Fill or remove empty Italian translation entries before merge.
These keys currently map to "", which can render blank controls/messages and weaken locale fallback behavior. Please add proper Italian copy (preferred) or remove the keys until translation is ready.
Also applies to: 87-87, 636-636, 640-642, 644-644, 650-650, 1105-1105, 1107-1107, 1128-1128, 1183-1184, 1223-1223, 1225-1225, 1233-1233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/messages/it.json` at line 68, Several Italian translation entries are
empty (e.g., the key "erYzJH" and also keys at positions corresponding to the
other empty entries listed); either provide proper Italian copy for each empty
JSON key or remove the key entirely so the app falls back to the default locale.
Update the JSON keys "erYzJH" and the other empty keys (the ones reported at 87,
636, 640-642, 644, 650, 1105, 1107, 1128, 1183-1184, 1223, 1225, 1233) with
valid Italian strings or delete those key entries to avoid rendering blank UI
text.
| @@ -0,0 +1,427 @@ | |||
| "use strict"; | |||
|
|
|||
| const SDK_VERSION = "0.1.0"; | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Keep SDK version in sync with package.json.
The SDK_VERSION constant is hardcoded and will drift from package.json version field unless manually updated on every release.
♻️ Recommended fix: Import version from package.json
-const SDK_VERSION = "0.1.0";
+const { version: SDK_VERSION } = require("./package.json");This ensures the version is always consistent with the published package.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@react-native/index.js` at line 3, The SDK_VERSION constant in
react-native/index.js is hardcoded and can drift from package.json; replace the
literal SDK_VERSION with the package version by importing the version field from
package.json (e.g., require or import the JSON and use its .version) and assign
that to SDK_VERSION so it always matches package.json; update any references to
SDK_VERSION to use the new imported value.
Comment on lines
+204
to
+227
| createBasePayload(context) { | ||
| this.ensureInitialized(); | ||
| const screenSize = getScreenSize(); | ||
| const appIdentifier = context?.appIdentifier || this.config.appIdentifier || this.config.siteId; | ||
|
|
||
| const payload = { | ||
| site_id: this.config.siteId, | ||
| anonymous_id: this.anonymousId, | ||
| hostname: appIdentifier, | ||
| pathname: asPathname(context?.pathname || context?.screen || "/"), | ||
| querystring: context?.querystring || "", | ||
| screenWidth: screenSize.screenWidth, | ||
| screenHeight: screenSize.screenHeight, | ||
| language: context?.language || getLanguage(), | ||
| page_title: context?.title || context?.screen || "", | ||
| referrer: context?.referrer || "", | ||
| user_agent: context?.userAgent || getUserAgent(this.config.appVersion), | ||
| }; | ||
|
|
||
| if (this.userId) payload.user_id = this.userId; | ||
| if (this.config.tag) payload.tag = this.config.tag; | ||
|
|
||
| return payload; | ||
| } |
Contributor
There was a problem hiding this comment.
Consider requiring appIdentifier for mobile site tracking.
Line 207 falls back to this.config.siteId when appIdentifier is missing, then uses it as the hostname field (line 212). For mobile applications, using a numeric site ID as the hostname is semantically incorrect and could lead to confusing analytics data.
Mobile sites should require a proper app identifier (e.g., com.example.app or bundle ID). Consider validating that appIdentifier is set during init() or at the server layer that distinguishes mobile from web sites.
💡 Suggested validation in init()
async init(config) {
if (!config || !config.analyticsHost || !config.siteId) {
throw new Error("analyticsHost and siteId are required");
}
+ if (!config.appIdentifier && !config.bundleId) {
+ throw new Error("appIdentifier or bundleId is required for React Native SDK");
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@react-native/index.js` around lines 204 - 227, createBasePayload currently
falls back to this.config.siteId for the hostname which can put numeric site IDs
into the hostname field; enforce a proper app identifier by (1) changing init()
to validate that this.config.appIdentifier (or context.appIdentifier) is present
for mobile/React Native usage and throw or log a clear error if missing, and (2)
updating createBasePayload to use only context.appIdentifier ||
this.config.appIdentifier for hostname (do not fallback to this.config.siteId);
reference createBasePayload and init (or the module initialization path) and
update any docs/tests that construct the client so mobile integrations supply a
bundle/app identifier like com.example.app.
Comment on lines
+70
to
+80
| const appIdentifierRegex = /^[A-Za-z0-9][A-Za-z0-9._-]{0,252}$/; | ||
| if (siteType === "web" && !domainRegex.test(cleanedDomain)) { | ||
| return reply.status(400).send({ | ||
| error: "Invalid domain format. Must be a valid domain like example.com or sub.example.com", | ||
| }); | ||
| } | ||
| if (siteType === "mobile" && !appIdentifierRegex.test(cleanedDomain)) { | ||
| return reply.status(400).send({ | ||
| error: "Invalid app identifier. Use a bundle/package identifier like com.example.app", | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Tighten mobile app identifier validation to match bundle/package format.
Line 70 currently accepts many invalid identifiers (e.g., single-token values like myapp or trailing-dot variants) despite the error message requiring a bundle/package-style identifier. This can persist invalid mobile site identifiers.
Suggested fix
- const appIdentifierRegex = /^[A-Za-z0-9][A-Za-z0-9._-]{0,252}$/;
+ // Require dot-separated bundle/package style (e.g. com.example.app)
+ const appIdentifierRegex =
+ /^(?=.{3,253}$)[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*(?:\.[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*)+$/;📝 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.
Suggested change
| const appIdentifierRegex = /^[A-Za-z0-9][A-Za-z0-9._-]{0,252}$/; | |
| if (siteType === "web" && !domainRegex.test(cleanedDomain)) { | |
| return reply.status(400).send({ | |
| error: "Invalid domain format. Must be a valid domain like example.com or sub.example.com", | |
| }); | |
| } | |
| if (siteType === "mobile" && !appIdentifierRegex.test(cleanedDomain)) { | |
| return reply.status(400).send({ | |
| error: "Invalid app identifier. Use a bundle/package identifier like com.example.app", | |
| }); | |
| } | |
| const appIdentifierRegex = /^[A-Za-z0-9][A-Za-z0-9._-]{0,252}$/; | |
| // Require dot-separated bundle/package style (e.g. com.example.app) | |
| const appIdentifierRegex = | |
| /^(?=.{3,253}$)[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*(?:\.[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*)+$/; | |
| if (siteType === "web" && !domainRegex.test(cleanedDomain)) { | |
| return reply.status(400).send({ | |
| error: "Invalid domain format. Must be a valid domain like example.com or sub.example.com", | |
| }); | |
| } | |
| if (siteType === "mobile" && !appIdentifierRegex.test(cleanedDomain)) { | |
| return reply.status(400).send({ | |
| error: "Invalid app identifier. Use a bundle/package identifier like com.example.app", | |
| }); | |
| } |
Suggested change
| const appIdentifierRegex = /^[A-Za-z0-9][A-Za-z0-9._-]{0,252}$/; | |
| if (siteType === "web" && !domainRegex.test(cleanedDomain)) { | |
| return reply.status(400).send({ | |
| error: "Invalid domain format. Must be a valid domain like example.com or sub.example.com", | |
| }); | |
| } | |
| if (siteType === "mobile" && !appIdentifierRegex.test(cleanedDomain)) { | |
| return reply.status(400).send({ | |
| error: "Invalid app identifier. Use a bundle/package identifier like com.example.app", | |
| }); | |
| } | |
| // Require dot-separated bundle/package style (e.g. com.example.app) | |
| const appIdentifierRegex = | |
| /^(?=.{3,253}$)[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*(?:\.[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*)+$/; | |
| if (siteType === "web" && !domainRegex.test(cleanedDomain)) { | |
| return reply.status(400).send({ | |
| error: "Invalid domain format. Must be a valid domain like example.com or sub.example.com", | |
| }); | |
| } | |
| if (siteType === "mobile" && !appIdentifierRegex.test(cleanedDomain)) { | |
| return reply.status(400).send({ | |
| error: "Invalid app identifier. Use a bundle/package identifier like com.example.app", | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/sites/addSite.ts` around lines 70 - 80, The current
appIdentifierRegex is too permissive and allows single-token or trailing-dot
values; update the regex used in the siteType === "mobile" validation (symbol:
appIdentifierRegex and variable cleanedDomain in addSite.ts) to enforce
bundle/package format: require at least two dot-separated components, no
leading/trailing/consecutive dots, each component starting with a letter and
containing only letters, digits or underscores/hyphens as allowed (e.g. replace
with something like ^[A-Za-z][A-Za-z0-9_-]*(?:\.[A-Za-z][A-Za-z0-9_-]*)+$), then
use that regex in the existing if (siteType === "mobile" &&
!appIdentifierRegex.test(cleanedDomain)) branch and update the error message to
mention the required dot-separated bundle identifier format.
Comment on lines
+87
to
+88
| const nextDomain = updateData.domain ?? site.domain; | ||
| const cleanedDomain = nextDomain.replace(/^https?:\/\//, "").replace(/\/+$/, ""); |
Contributor
There was a problem hiding this comment.
Guard nullish stored domains before calling replace().
site.domain can still be nullish for existing rows, and this normalization now runs on every update before the later domain/type guard. Line 88 will throw on a valid PATCH that only changes an unrelated field, turning it into a 500.
Suggested fix
- const nextDomain = updateData.domain ?? site.domain;
+ const nextDomain = updateData.domain ?? site.domain ?? "";
const cleanedDomain = nextDomain.replace(/^https?:\/\//, "").replace(/\/+$/, "");📝 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.
Suggested change
| const nextDomain = updateData.domain ?? site.domain; | |
| const cleanedDomain = nextDomain.replace(/^https?:\/\//, "").replace(/\/+$/, ""); | |
| const nextDomain = updateData.domain ?? site.domain ?? ""; | |
| const cleanedDomain = nextDomain.replace(/^https?:\/\//, "").replace(/\/+$/, ""); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/sites/updateSiteConfig.ts` around lines 87 - 88, The code uses
nextDomain = updateData.domain ?? site.domain and immediately calls replace() on
it, which can throw when site.domain is nullish; update the logic around
nextDomain/cleanedDomain in updateSiteConfig.ts to guard for null/undefined
before calling replace(): compute nextDomain as before, then if nextDomain is
nullish skip normalization (e.g., leave cleanedDomain undefined or null) or
normalize via a safe fallback like (nextDomain ?? "") before replace, and ensure
the later domain/type validation still rejects absent domains as intended;
update references to nextDomain and cleanedDomain accordingly so unrelated
PATCHes don't hit replace() on nullish site.domain.
Comment on lines
+70
to
+74
| async generateUserIdFromClientId(clientId: string, siteId: number): Promise<string> { | ||
| const config = await siteConfig.getConfig(siteId); | ||
| const salt = config?.saltUserIds ? this.getDailySalt() : ""; | ||
|
|
||
| return crypto.createHash("sha256").update(`${siteId}:${clientId}:${salt}`).digest("hex").substring(0, 12); |
Contributor
There was a problem hiding this comment.
Keep client-derived IDs stable across days.
Applying getDailySalt() here makes the same anonymous_id hash differently each UTC day when saltUserIds is enabled. That breaks the new identify flow across day boundaries: yesterday's anonymous events are stored under one hash, today's identify call computes another, and the backfill can never stitch them together. Daily rotation makes sense for the IP/UA fallback, but not for a client-supplied stable ID.
Suggested fix
async generateUserIdFromClientId(clientId: string, siteId: number): Promise<string> {
- const config = await siteConfig.getConfig(siteId);
- const salt = config?.saltUserIds ? this.getDailySalt() : "";
-
- return crypto.createHash("sha256").update(`${siteId}:${clientId}:${salt}`).digest("hex").substring(0, 12);
+ return crypto.createHash("sha256").update(`${siteId}:${clientId}`).digest("hex").substring(0, 12);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/services/userId/userIdService.ts` around lines 70 - 74, The
generateUserIdFromClientId function is using getDailySalt(), causing
client-derived anonymous_id hashes to change each UTC day; replace the
daily-rotating salt with a stable site-specific salt so the same clientId always
hashes to the same user id across days. Update generateUserIdFromClientId to use
a persistent salt from the site config (e.g., a config.userIdSalt or
config.salt) or an empty string when saltUserIds is false instead of calling
this.getDailySalt(), ensuring the hash input remains stable; adjust any
references to getDailySalt() here to a stable-salt accessor or field.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores