OCV refactor #10388
OCV refactor #10388
Conversation
srietkerk
requested review from
Copilot and
a team
and removed request for
a team
There was a problem hiding this comment.
PR Overview
This PR refactors the telemetry and feedback features for OCV by introducing an ocv configuration object and adjusting the feedback components to operate without a modal. Key changes include:
- Replacing the Boolean ocvEnabled flag with an ocv config object (appId and iframeEndpoint) across pxtlib and webapp.
- Refactoring the feedback components by splitting them into FeedbackModal and Feedback for better separation of concerns.
- Updating browser utilities and telemetry handling to align with the new OCV configuration.
Reviewed Changes
| File | Description |
|---|---|
| pxtlib/browserutils.ts | Updated regex in isLocalHost to allow both HTTP and HTTPS. |
| react-common/components/controls/Feedback/Feedback.tsx | Refactored feedback components, renaming and reordering. |
| pxtlib/main.ts | Replaced ocvEnabled flag with an ocv config object. |
| react-common/components/controls/Feedback/FeedbackEventListener.ts | Updated telemetry configuration and ocv references. |
| webapp/src/container.tsx, webapp/src/projects.tsx, webapp/src/app.tsx | Updated feature flag checks to use the new ocv config fields. |
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
react-common/components/controls/Feedback/Feedback.tsx:23
- [nitpick] The roles of the FeedbackModal and the inner Feedback components appear similar and may cause confusion; consider renaming the inner Feedback component (e.g., to FeedbackContent) or adding comments to clearly distinguish their responsibilities.
export const FeedbackModal = (props: IFeedbackModalProps) => {
| const { kind, onClose } = props; | ||
| const title = kind === "generic" ? lf("Leave Feedback") : lf("Rate this activity"); |
There was a problem hiding this comment.
nit: I would swap this to check if kind === "rating" so the fallback is generic (i.e. "Leave Feedback") in the event of an unrecognized/new kind being introduced.
| <iframe | ||
| title="feedback" | ||
| id={frameId} | ||
| src={`${pxt.appTarget.appTheme.ocvFrameUrl}/centrohost?appname=ocvfeedback&feature=host-ocv-inapp-feedback&platform=web&appId=${pxt.appTarget.appTheme.ocvAppId}#/hostedpage`} | ||
| src={`${pxt.webConfig.ocv?.iframeEndpoint}/centrohost?appname=ocvfeedback&feature=host-ocv-inapp-feedback&platform=web&appId=${pxt.webConfig.ocv?.appId}#/hostedpage`} |
There was a problem hiding this comment.
I'm wondering if, rather than ocv?., we should we check src={ocv ? "..." : undefined}. What actually happens if ocv is null here and this becomes a malformed URL?
There was a problem hiding this comment.
Oh, that's an interesting idea. Technically, in order to have this component be active at all it has to go through the menus where we already have checks to make sure that the ocv object exists. We might just want to do a check for the ocv object before the iframe in this case, because I agree, we wouldn't want a malformed url here.
| @@ -307,7 +307,7 @@ export class ProjectSettingsMenu extends data.Component<ProjectSettingsMenuProps | |||
| const githubUser = !hasIdentity && this.getData("github:user") as UserInfo; | |||
| const reportAbuse = pxt.appTarget.cloud && pxt.appTarget.cloud.sharing && pxt.appTarget.cloud.importing; | |||
| const showDivider = targetTheme.selectLanguage || targetTheme.highContrast || githubUser; | |||
| const showFeedbackOption = pxt.webConfig.ocvEnabled && targetTheme.feedbackEnabled && targetTheme.ocvFrameUrl && targetTheme.ocvAppId; | |||
| const showFeedbackOption = pxt.webConfig.ocv?.appId && pxt.webConfig.ocv?.iframeEndpoint; | |||
There was a problem hiding this comment.
Since we do this check, I wonder if it'd be nice to have a little helper function for ocvEnabled() somewhere, maybe in the ocv namespace? Not a big deal either way.
There was a problem hiding this comment.
Ah, yeah, good idea. I definitely found it annoying to type this out every where, I'll add that in.
| <Modal className="feedback-modal" title={title} onClose={onClose}> | ||
| <> | ||
| { kind === "generic" && | ||
| <Feedback |
There was a problem hiding this comment.
If the differences between these two feedback components are fully based on the kind field, and if that kind field has to be passed into the Feedback component anyway, what's the motivation to split them here rather than having the kind-based checks inside the Feedback component itself? (In other words, could we set frameId inside Feedback based on kind? Then I don't think we'd need this split creating different Feedback components based on it).
Another approach would just be to have:
const frameId = kind === "rating" ? "activity-feedback-frame" : "menu-feedback-frame";
<Feedback kind={kind} frameId={frameId} onClose={onClose} />
Added some device telemetry points to see in OCV.
Refactored the feedback it so it's possible to have the feedback without the modal.
Made it so feedback would only show up if we're getting the right data from webconfig.