feat(web): update story custom domain extension #1361
Conversation
WalkthroughThe pull request introduces modifications to handle project and story types more dynamically across multiple files. The changes primarily focus on adding a Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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 (5)
web/src/services/api/storytellingApi/utils.ts (1)
46-46: Consider making__typenameoptional.The
__typenameproperty should be marked as optional with?modifier since it's typically added by GraphQL automatically and may not always be present when creatingStoryobjects manually.- __typename: string; + __typename?: string;web/src/services/config/extensions.ts (1)
32-39: Extract common fields into a base type.Both
ProjectPublicationExtensionPropsandStoryPublicationExtensionPropsshare common fields. Consider extracting these into a base type to reduce duplication and improve maintainability.+type BasePublicationExtensionProps = { + publishDisabled?: boolean; + typename: string; +} & SharedExtensionProps; -export type ProjectPublicationExtensionProps = { +export type ProjectPublicationExtensionProps = BasePublicationExtensionProps & { projectId: string; projectAlias?: string; - publishDisabled?: boolean; - typename: string; -} & SharedExtensionProps; }; -export type StoryPublicationExtensionProps = { +export type StoryPublicationExtensionProps = BasePublicationExtensionProps & { storyId: string; storyAlias?: string; - publishDisabled?: boolean; - typename: string; -} & SharedExtensionProps; };web/src/beta/features/ProjectSettings/innerPages/PublicSettings/index.tsx (1)
55-55: Consider making__typenameoptional.Similar to the
Storytype, the__typenameproperty should be marked as optional with?modifier since it may not always be present when creatingSettingsProjectobjects manually.- __typename: string; + __typename?: string;web/src/beta/features/ProjectSettings/innerPages/PublicSettings/PublicSettingsDetail.tsx (2)
111-121: Consider making the extension ID more configurable.The
extensionIdis currently hardcoded ascustom-${type}-domain. Consider making it more flexible by accepting it as a prop or configuration.- const extensionId = `custom-${type}-domain`; + const extensionId = props.extensionId ?? `custom-${type}-domain`;
267-267: Consider making the version configurable.The version is hardcoded as "visualizer". Consider making it configurable through environment variables or props.
- version={"visualizer"} + version={process.env.REACT_APP_VERSION ?? "visualizer"}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/PublicSettingsDetail.tsx(3 hunks)web/src/beta/features/ProjectSettings/innerPages/PublicSettings/index.tsx(1 hunks)web/src/services/api/storytellingApi/utils.ts(1 hunks)web/src/services/config/extensions.ts(2 hunks)
| {extensions && | ||
| extensions.length > 0 && | ||
| settingsItem.__typename && | ||
| accessToken && ( | ||
| <Collapse title={t("Custom Domain")} size="large"> | ||
| {settingsItem.__typename === "Project" && ( | ||
| <ExtensionComponent | ||
| typename={settingsItem.__typename} | ||
| key={settingsItem.id} | ||
| projectId={settingsItem.id} | ||
| projectAlias={settingsItem.alias} | ||
| lang={currentLang} | ||
| theme={currentTheme} | ||
| accessToken={accessToken} | ||
| onNotificationChange={() => onNotificationChange} | ||
| version={"visualizer"} | ||
| /> | ||
| )} | ||
| {settingsItem.__typename === "Story" && ( | ||
| <ExtensionComponent | ||
| typename={settingsItem.__typename} | ||
| key={settingsItem.id} | ||
| storyId={settingsItem.id} | ||
| storyAlias={settingsItem.alias} | ||
| lang={currentLang} | ||
| theme={currentTheme} | ||
| accessToken={accessToken} | ||
| onNotificationChange={() => onNotificationChange} | ||
| version={"visualizer"} | ||
| /> | ||
| )} | ||
| </Collapse> | ||
| )} |
There was a problem hiding this comment.
Fix incorrect callback and reduce code duplication.
- The
onNotificationChangeprop is incorrectly passed as a function that returns a function. - There's significant duplication between Project and Story extension components.
Consider this refactoring:
{extensions &&
extensions.length > 0 &&
settingsItem.__typename &&
accessToken && (
<Collapse title={t("Custom Domain")} size="large">
- {settingsItem.__typename === "Project" && (
- <ExtensionComponent
- typename={settingsItem.__typename}
- key={settingsItem.id}
- projectId={settingsItem.id}
- projectAlias={settingsItem.alias}
- lang={currentLang}
- theme={currentTheme}
- accessToken={accessToken}
- onNotificationChange={() => onNotificationChange}
- version={"visualizer"}
- />
- )}
- {settingsItem.__typename === "Story" && (
- <ExtensionComponent
- typename={settingsItem.__typename}
- key={settingsItem.id}
- storyId={settingsItem.id}
- storyAlias={settingsItem.alias}
- lang={currentLang}
- theme={currentTheme}
- accessToken={accessToken}
- onNotificationChange={() => onNotificationChange}
- version={"visualizer"}
- />
- )}
+ <ExtensionComponent
+ typename={settingsItem.__typename}
+ key={settingsItem.id}
+ {...(settingsItem.__typename === "Project"
+ ? {
+ projectId: settingsItem.id,
+ projectAlias: settingsItem.alias,
+ }
+ : {
+ storyId: settingsItem.id,
+ storyAlias: settingsItem.alias,
+ })}
+ lang={currentLang}
+ theme={currentTheme}
+ accessToken={accessToken}
+ onNotificationChange={onNotificationChange}
+ version="visualizer"
+ />
</Collapse>
)}📝 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.
| {extensions && | |
| extensions.length > 0 && | |
| settingsItem.__typename && | |
| accessToken && ( | |
| <Collapse title={t("Custom Domain")} size="large"> | |
| {settingsItem.__typename === "Project" && ( | |
| <ExtensionComponent | |
| typename={settingsItem.__typename} | |
| key={settingsItem.id} | |
| projectId={settingsItem.id} | |
| projectAlias={settingsItem.alias} | |
| lang={currentLang} | |
| theme={currentTheme} | |
| accessToken={accessToken} | |
| onNotificationChange={() => onNotificationChange} | |
| version={"visualizer"} | |
| /> | |
| )} | |
| {settingsItem.__typename === "Story" && ( | |
| <ExtensionComponent | |
| typename={settingsItem.__typename} | |
| key={settingsItem.id} | |
| storyId={settingsItem.id} | |
| storyAlias={settingsItem.alias} | |
| lang={currentLang} | |
| theme={currentTheme} | |
| accessToken={accessToken} | |
| onNotificationChange={() => onNotificationChange} | |
| version={"visualizer"} | |
| /> | |
| )} | |
| </Collapse> | |
| )} | |
| {extensions && | |
| extensions.length > 0 && | |
| settingsItem.__typename && | |
| accessToken && ( | |
| <Collapse title={t("Custom Domain")} size="large"> | |
| <ExtensionComponent | |
| typename={settingsItem.__typename} | |
| key={settingsItem.id} | |
| {...(settingsItem.__typename === "Project" | |
| ? { | |
| projectId: settingsItem.id, | |
| projectAlias: settingsItem.alias, | |
| } | |
| : { | |
| storyId: settingsItem.id, | |
| storyAlias: settingsItem.alias, | |
| })} | |
| lang={currentLang} | |
| theme={currentTheme} | |
| accessToken={accessToken} | |
| onNotificationChange={onNotificationChange} | |
| version="visualizer" | |
| /> | |
| </Collapse> | |
| )} |
There was a problem hiding this comment.
@hexaforce Thanks for confirming! I'll mark this as resolved.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/PublicSettingsDetail.tsx (2)
111-121: Consider enhancing type safety for the extension component.While the implementation is correct, we can improve type safety:
const ExtensionComponent = ( props: ProjectPublicationExtensionProps | StoryPublicationExtensionProps ) => { - const type = props.typename.toLocaleLowerCase(); + const type = props.typename.toLowerCase() === 'project' ? 'project' : 'story'; const extensionId = `custom-${type}-domain`; const Component = extensions?.find((e) => e.id === extensionId)?.component; if (!Component) { return null; } return <Component {...props} />; };This change ensures that only valid types ('project' or 'story') are used in the extension ID.
273-273: Consider making the version configurable.The version is hardcoded as "visualizer". Consider making it configurable through props or environment variables for better flexibility.
- version="visualizer" + version={process.env.REACT_APP_VISUALIZER_VERSION ?? "visualizer"}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/PublicSettingsDetail.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (2)
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/PublicSettingsDetail.tsx (2)
8-11: LGTM! Clean import structure.The new type imports are well-organized and provide proper type safety for the extension components.
252-276: Great implementation following previous review suggestions!The code successfully implements the recommended approach for reducing duplication while maintaining type safety.
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/PublicSettingsDetail.tsx (1)
273-273: Consider making the version configurable.The version is hardcoded as "visualizer". Consider making it configurable through props or environment variables for better flexibility.
- version="visualizer" + version={process.env.REACT_APP_VISUALIZER_VERSION ?? "visualizer"}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/PublicSettingsDetail.tsx(3 hunks)web/src/beta/features/ProjectSettings/innerPages/PublicSettings/index.tsx(1 hunks)web/src/services/api/storytellingApi/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/beta/features/ProjectSettings/innerPages/PublicSettings/index.tsx
- web/src/services/api/storytellingApi/utils.ts
🔇 Additional comments (3)
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/PublicSettingsDetail.tsx (3)
8-11: LGTM! Clean import structure.The new imports for extension types properly separate project and story publication props, aligning with the PR objectives.
111-121: Well-structured component implementation!The ExtensionComponent elegantly handles both project and story extensions with proper null checks and dynamic component resolution.
252-276: Clean implementation resolving previous duplication issues!The conditional rendering and type-safe props spreading effectively addresses the code duplication mentioned in previous reviews.
Co-authored-by: Tomokazu Tantaka <[email protected]>
Overview
Added custom domain for stories.
What I've done
I have added a new extension to reearth-cloud.js.
https://github.com/eukarya-inc/reearth-cloud/pull/95/files#diff-a4fc01cb110b1ca5c89a7624b5c8a4a455f52bd40eddfc554632625665b61ae4
As a result, it is necessary to update the extension of reearth-visualizer:
{ type: "publication", - id: "custom-domain", + id: "custom-project-domain", component: CustomDomainSection, }, +{ + type: "publication", + id: "custom-story-domain", + component: CustomStoryDomainSection, +},Currently, both use the custom-domain extension, but I have modified it so that the custom-story-domain is used for stories.
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Improvements
typenameproperty to support more precise type identification.