Conversation
46efc2e to
6c1df18
Compare
|
Moving as draft because I'm waiting for Fullstory is activated on writer.com |
![hackerone-code[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit 6c1df18 (latest)
WalkthroughThis change introduces FullStory analytics integration into the Writer Cloud app, enabling session recording when in cloud and edit mode. It adds FullStory dependencies, implements dynamic loading and initialization, expands event tracking to support both API and FullStory, and removes the previous Chameleon integration. Supporting utilities and tests are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant WriterApi
participant FullStory
participant EnvConfig
User->>App: Mounts app in cloud & edit mode
App->>WriterApi: fetchUserProfile()
App->>EnvConfig: getWriterCloudEnvConfig()
App->>FullStory: Dynamically import & initialize with orgId
App->>FullStory: Identify user with profile
App->>WriterApi: analyticsIdentify(user)
App->>App: User triggers event
App->>WriterApi: analyticsTrack(event, props)
App->>FullStory: trackEventAsync(event, props)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit 338c7a9 (latest)
65ab3a9 to
91aee0c
Compare
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit 91aee0c (latest)
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit b1e73cc (latest)
![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
🧹 Nitpick comments (2)
src/ui/src/utils/writerCloudEnvConfig.ts (1)
32-76: Security consideration: Dynamic import of remote JavaScript.While the implementation includes proper error handling and resource cleanup, loading and executing remote JavaScript via dynamic imports poses potential security risks. However, the risk is mitigated since:
- The URL is constructed from trusted sources (same origin or environment variable)
- It uses
credentials: "include"which suggests same-origin requests- Comprehensive error handling prevents failures from breaking the app
Consider documenting the security assumptions and ensuring the
/env/config.jsendpoint is properly secured on the server side.src/ui/src/composables/useWriterTracking.ts (1)
136-151: Remove redundant canTrack checks.The
canTrackcheck intrack()makes the checks intrackWithFullStoryandtrackWithApiredundant.async function trackWithFullStory( eventName: string, properties: EventProperties, ) { - if (!canTrack.value) return; const fullstoryOrgId = await getFullstoryOrgId(); if (!fullstoryOrgId) return; const { FullStory } = await import("@fullstory/browser"); return FullStory("trackEventAsync", { name: eventName, properties }); } function trackWithApi(eventName: string, properties: EventProperties) { - if (!canTrack.value) return; return writerApi.analyticsTrack(eventName, properties); }Also applies to: 182-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
src/ui/package.json(1 hunks)src/ui/src/builder/BuilderSwitcher.spec.ts(0 hunks)src/ui/src/composables/useCollaborationManager.ts(1 hunks)src/ui/src/composables/useWriterTracking.spec.ts(1 hunks)src/ui/src/composables/useWriterTracking.ts(4 hunks)src/ui/src/core/useSourceFiles.ts(1 hunks)src/ui/src/main.ts(1 hunks)src/ui/src/utils/writerCloudEnvConfig.ts(1 hunks)src/ui/src/writerApi.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/ui/src/builder/BuilderSwitcher.spec.ts
- src/ui/src/writerApi.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/ui/src/composables/useWriterTracking.spec.ts (1)
src/ui/src/writerApi.ts (3)
fetchUserProfile(113-119)analyticsIdentify(129-137)analyticsTrack(139-150)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: build (3.10)
- GitHub Check: build (3.9)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.12)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.9)
- GitHub Check: tests (firefox)
- GitHub Check: tests (webkit)
- GitHub Check: tests (chromium)
🔇 Additional comments (13)
src/ui/package.json (1)
26-26: FullStory Dependency Version ConfirmedThe
@fullstory/browserdependency at version^2.0.6is the latest stable release, and there are no known security advisories affecting it. Keep the caret range to allow safe minor/patch updates as usual.src/ui/src/core/useSourceFiles.ts (1)
160-164: LGTM! Clean extension mapping for JavaScript and TypeScript.The language detection logic correctly maps JavaScript and TypeScript file extensions to their respective language identifiers, following the established pattern used for other languages.
src/ui/src/utils/writerCloudEnvConfig.ts (3)
15-27: Good error handling for remote config fetching.The error handling in
getWriterCloudEnvConfigTextis robust - it properly catches fetch failures and returnsundefinedinstead of throwing, which allows the calling code to handle missing configs gracefully.
46-59: Proper resource management with blob URLs.The implementation correctly creates and cleans up blob URLs, ensuring no memory leaks. The use of
finallyblock guarantees cleanup even if the import fails.
61-73: Robust config format handling.The code properly handles multiple config formats (object, base64-encoded string) with appropriate fallbacks. The use of
atob()for base64 decoding and proper JSON parsing with error handling is well implemented.src/ui/src/composables/useCollaborationManager.ts (1)
47-47: Good defensive programming with comprehensive null checks.The expanded guard clause now properly handles both
undefinedandnullvalues forincomingUserId, making the collaboration system more robust against edge cases.src/ui/src/main.ts (1)
66-66: Excellent error handling improvement.Adding
.catch(logger.error)ensures that any failures in collaboration enablement are properly logged rather than causing unhandled promise rejections. This follows the same pattern used forsecretsManager.load()and improves application reliability.src/ui/src/composables/useWriterTracking.spec.ts (1)
9-9: LGTM! Mock updates align with implementation changes.The mock updates correctly reflect the refactored implementation:
- Replaced
loadAndIdentifyChameleonwithfetchUserProfile- Added mock for
getWriterCloudEnvConfigreturning empty object, which prevents FullStory initialization in testsAlso applies to: 15-15, 19-22
src/ui/src/composables/useWriterTracking.ts (5)
57-67: Good refactoring with centralized tracking condition.The
canTrackcomputed property effectively centralizes the tracking logic, andgetFullstoryOrgIdsafely retrieves the organization ID with proper cloud app validation.
74-79: Excellent optimization with shared promise.Smart use of Promise.all for parallel execution and sharing the
fetchUserProfilepromise between initialization functions to avoid duplicate API calls.
153-159: Clean extraction of component information logic.Good refactoring that improves code organization and handles edge cases properly.
89-106: Critical: Remove hardcoded API key from source code.The Chameleon API key is hardcoded in the source code, which is a security risk. API keys should be loaded from environment configuration.
async function initializeChameleon( fetchUserProfile: Promise<WriterApiUserProfile>, ) { + const config = await getWriterCloudEnvConfig(); + const chameleonApiKey = config["CHAMELEON_API_KEY"]; + if (!chameleonApiKey) { + logger.warn("Chameleon API key not found in environment config"); + return; + } + const chameleon = await import("@chamaeleonidae/chmln"); chameleon.init( - "S6dr31v4MO4wuteztVhxysLkoA9HsZ6HnDaOTDLqNXHYZq-1P02ek-E3BR4Zgv4pMwdoYO", + String(chameleonApiKey), { fastUrl: "https://fast.chameleon.io/", }, ); const profile = await fetchUserProfile; chameleon.identify(profile.id, { email: profile?.email, name: profile?.fullName ?? `${profile?.firstName} ${profile.lastName}`, }); }⛔ Skipped due to learnings
Learnt from: ramedina86 PR: writer/writer-framework#974 File: src/ui/src/writerApi.ts:141-155 Timestamp: 2025-06-24T14:00:42.917Z Learning: Client-side API keys for user onboarding services like Chameleon are meant to be publicly visible in the client bundle and are not security issues when hardcoded in client-side code.
199-210: Keep the explicit writerOrgId checkThe
canTrackguard ensuresmode === "edit"andisWriterCloudApp(which is true if eitherwriterAppIdorwriterOrgIdis defined), butwriterOrgIdcan still be undefined when onlywriterAppIdexists. The extraif (!wf.writerOrgId.value)prevents callinganalyticsPagewithout a valid org ID. No change needed.
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit 7614faf (latest)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/ui/src/composables/useWriterTracking.ts (3)
132-132: This duplicates a previous review concern - verify the segment prefix necessity.The literal
"segment-prefix-"may not be necessary for your integration.Please confirm whether this prefix is required by your Segment analytics integration. If not needed, remove it:
-uid: `segment-prefix-${profile.id}`, +uid: String(profile.id),
123-127: This duplicates a previous review concern - add error handling for FullStory initialization.The
module.initcall lacks proper error handling, which could lead to unhandled promise rejections.Wrap the initialization in a try-catch block:
-module.init({ - orgId: fullstoryOrgId, - // @ts-expect-error importing vite variable - devMode: import.meta.env.DEV, -}); +try { + module.init({ + orgId: fullstoryOrgId, + devMode: import.meta.env.DEV, + }); +} catch (e) { + logger.error("Failed to initialize FullStory", e); + return; +}
125-126: This duplicates a previous review concern - fix TypeScript error suppression.The
@ts-expect-errorindicates missing Vite type definitions in your TypeScript configuration.Add Vite types to your
tsconfig.json:{ "compilerOptions": { "types": ["vite/client"] } }
🧹 Nitpick comments (2)
src/ui/src/composables/useWriterTracking.ts (2)
60-66: Consider caching the config result.The function correctly handles the Writer Cloud app check, but multiple calls to
getWriterCloudEnvConfig()could impact performance since this is called in multiple places.Consider caching the config result:
+let configCache: Promise<Record<string, unknown>> | null = null; + async function getFullstoryOrgId() { if (!wf.isWriterCloudApp.value) return undefined; - const config = await getWriterCloudEnvConfig(); + if (!configCache) { + configCache = getWriterCloudEnvConfig(); + } + const config = await configCache; return config["FULLSTORY_ORG_ID"] ? String(config["FULLSTORY_ORG_ID"]) : undefined; }
198-201: Consider independent tracking failure handling.The current
Promise.allapproach means if one tracking provider fails, both fail. This could reduce tracking reliability.Consider allowing independent failures:
-return await Promise.all([ - trackWithApi(eventNameFormated, propertiesExpanded), - trackWithFullStory(eventNameFormated, propertiesExpanded), -]).catch(logger.error); +const results = await Promise.allSettled([ + trackWithApi(eventNameFormated, propertiesExpanded), + trackWithFullStory(eventNameFormated, propertiesExpanded), +]); + +results.forEach((result, index) => { + if (result.status === 'rejected') { + const provider = index === 0 ? 'API' : 'FullStory'; + logger.error(`${provider} tracking failed:`, result.reason); + } +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
src/ui/package.json(1 hunks)src/ui/src/builder/BuilderSwitcher.spec.ts(0 hunks)src/ui/src/components/core/embed/CorePDF.vue(0 hunks)src/ui/src/composables/useCollaborationManager.ts(1 hunks)src/ui/src/composables/useWriterApi.ts(0 hunks)src/ui/src/composables/useWriterTracking.spec.ts(1 hunks)src/ui/src/composables/useWriterTracking.ts(3 hunks)src/ui/src/core/useSourceFiles.ts(1 hunks)src/ui/src/main.ts(1 hunks)src/ui/src/utils/writerCloudEnvConfig.ts(1 hunks)src/ui/src/writerApi.ts(0 hunks)src/ui/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (4)
- src/ui/src/composables/useWriterApi.ts
- src/ui/src/components/core/embed/CorePDF.vue
- src/ui/src/builder/BuilderSwitcher.spec.ts
- src/ui/src/writerApi.ts
✅ Files skipped from review due to trivial changes (1)
- src/ui/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (6)
- src/ui/package.json
- src/ui/src/composables/useCollaborationManager.ts
- src/ui/src/main.ts
- src/ui/src/utils/writerCloudEnvConfig.ts
- src/ui/src/composables/useWriterTracking.spec.ts
- src/ui/src/core/useSourceFiles.ts
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.9)
- GitHub Check: tests (firefox)
- GitHub Check: tests (webkit)
- GitHub Check: tests (chromium)
- GitHub Check: build (3.9)
- GitHub Check: build (3.13)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
src/ui/src/composables/useWriterTracking.ts (4)
3-5: LGTM! Good reactive state management setup.The addition of Vue composition API imports and the new environment config utility properly supports the refactored reactive tracking logic.
56-58: Excellent centralized tracking condition.The
canTrackcomputed property provides a clean, reactive way to determine when tracking should be enabled, consolidating the edit mode and Writer Cloud app checks.
187-202: Good parallel tracking implementation.The async refactor and parallel tracking calls provide good performance while maintaining proper error logging. The conditional checks ensure tracking only occurs when appropriate.
98-103: Security concern: Hardcoded Chameleon key.The Chameleon initialization uses a hardcoded API key. This could be a security risk and makes environment management difficult.
Consider moving this to environment configuration:
async function initializeChameleon( fetchUserProfile = writerApi.fetchUserProfile(), ) { + const config = await getWriterCloudEnvConfig(); + const chameleonKey = config["CHAMELEON_API_KEY"]; + if (!chameleonKey) return; + const chameleon = await import("@chamaeleonidae/chmln"); chameleon.init( - "S6dr31v4MO4wuteztVhxysLkoA9HsZ6HnDaOTDLqNXHYZq-1P02ek-E3BR4Zgv4pMwdoYO", + String(chameleonKey), { fastUrl: "https://fast.chameleon.io/", }, );⛔ Skipped due to learnings
Learnt from: ramedina86 PR: writer/writer-framework#974 File: src/ui/src/writerApi.ts:141-155 Timestamp: 2025-06-24T14:00:42.917Z Learning: Client-side API keys for user onboarding services like Chameleon are meant to be publicly visible in the client bundle and are not security issues when hardcoded in client-side code.
There was a problem hiding this comment.
✅ This pull request was succesfully scanned and it was determined that it does not contain any changes that present immediate security concerns. If you would still like for it to be reviewed by an expert from our reviewer community, you can submit it manually via the HackerOne PullRequest dashboard.
⏱️ Latest scan covered changes up to commit 3a17fe2 (latest)
1 participant
Install
@fullstory/browserand initialize it inuseWriterTrackingWe load the Writer's env variables from
/env/config.js. It safely fails if the URL doesn't exist (like for local or self-hosted mode).Summary by CodeRabbit
New Features
Bug Fixes
Chores