fix(web): publish scene and story could use the same alias by mistake #1238
Conversation
WalkthroughThe pull request introduces several significant changes, primarily focusing on the management of publish statuses within the application. Key modifications include the centralization of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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. |
✅ 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: 13
🧹 Outside diff range and nitpick comments (43)
web/src/services/api/publishTypes.ts (2)
5-11: Consider improving type safety and readability.While the function works correctly, there are a few improvements that could be made:
- Use strict equality === consistently
- Consider using a switch statement for better readability
Here's a suggested refactor:
export const toGqlStatus = (status?: PublishStatus) => { - return status === "limited" - ? PublishmentStatus.Limited - : status == "published" - ? PublishmentStatus.Public - : PublishmentStatus.Private; + switch (status) { + case "limited": + return PublishmentStatus.Limited; + case "published": + return PublishmentStatus.Public; + default: + return PublishmentStatus.Private; + } };
13-22: Consider adding explicit return type annotation.The function implementation is clean and readable. Consider adding an explicit return type for better type safety and documentation.
-export const toPublishmentStatus = (s?: PublishmentStatus) => { +export const toPublishmentStatus = (s?: PublishmentStatus): PublishStatus => {web/src/beta/features/Editor/Publish/context.tsx (3)
6-6: Remove commented import statement.The commented import for
ProjectTypeshould be removed since it's no longer used. This helps maintain a clean codebase.-// import { ProjectType } from "./PublishToolsPanel/hooks";
12-16: Remove commented properties.Instead of keeping commented properties, they should be removed since they're no longer used. This improves code maintainability and readability.
handleVisualizerResize?: (props: AreaSize) => void; - // storyId?: string; projectId?: string; sceneId?: string; - // selectedProjectType?: ProjectType; - // handleProjectTypeChange: (type: ProjectType) => void;
8-11: Add JSDoc documentation for the interface.Please add documentation to explain the purpose of the
PublishPageContextTypeinterface and its properties, particularly howactiveSubProjecthelps prevent alias conflicts between scenes and stories.+/** + * Context type for the publish page functionality. + * Manages the active sub-project selection and visualization settings. + * @property activeSubProject - The currently selected sub-project (scene or story) + * @property handleActiveSubProjectChange - Callback to update the active sub-project + */ export interface PublishPageContextType { activeSubProject?: SubProject | undefined; handleActiveSubProjectChange?: (subProject: SubProject | undefined) => void;web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx (3)
4-13: Consider enhancing accessibility for disabled state.The disabled state is well-implemented visually, but could benefit from additional accessibility attributes.
Consider adding
aria-disabledattribute:export const Section = styled("div")<{ disabled?: boolean }>( ({ disabled, theme }) => ({ display: "flex", flexDirection: "column", padding: theme.spacing.normal, gap: theme.spacing.large, opacity: disabled ? 0.6 : 1, - cursor: disabled ? "not-allowed" : "auto" + cursor: disabled ? "not-allowed" : "auto", + "&[aria-disabled='true']": { + pointerEvents: "none" + } }) );
15-19: Consider adding semantic role for warning header.The header uses warning colors, suggesting it displays important messages.
Consider adding role and aria attributes:
export const Header = styled("div")(({ theme }) => ({ display: "flex", gap: theme.spacing.normal, - color: theme.warning.main + color: theme.warning.main, + "[role='alert']": { + margin: 0 + } }));
21-28: Consider using relative units for icon dimensions.Fixed pixel dimensions might not scale well across different screen sizes.
Consider using relative units:
export const WarningIcon = styled(Icon)({ - width: "24px", - height: "24px" + width: "1.5rem", + height: "1.5rem" });web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts (3)
45-54: Simplify alias validation logic and add cleanup.The validation logic is complex and could be simplified. Also, consider adding cleanup to prevent memory leaks.
useEffect(() => { + let mounted = true; setAliasValid( - !!publishItem.alias || - (!validatingAlias && - !!checkProjectAliasData && - (publishItem.alias === - checkProjectAliasData.checkProjectAlias.alias || - checkProjectAliasData.checkProjectAlias.available)) + Boolean( + publishItem.alias || + (!validatingAlias && + checkProjectAliasData?.checkProjectAlias?.available) + ) ); + return () => { mounted = false; }; }, [validatingAlias, checkProjectAliasData, publishItem]);
56-60: Add TypeScript return type definition.Consider adding explicit return type for better type safety and documentation.
+interface UseAliasResult { + alias: string | undefined; + aliasValid: boolean; +} + -export default ({ publishItem }: { publishItem: PublishItem }) => { +export default ({ publishItem }: { publishItem: PublishItem }): UseAliasResult => {
62-64: Consider inlining the generateAlias function.Since this function is a simple wrapper and only used once, consider inlining it to reduce code complexity.
-function generateAlias() { - return generateRandomString(10); -} const createAlias = useCallback(() => { createAliasCount.current += 1; if (createAliasCount.current > 3) return; - const newAlias = generateAlias(); + const newAlias = generateRandomString(10); setAlias(newAlias); checkProjectAlias({ variables: { alias: newAlias } }); }, [checkProjectAlias]);web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx (2)
17-22: Consider renaming hooks for clarity.The hooks
usePublishProjectandusePublishStoryare being used for unpublishing operations. This could be confusing. Consider either:
- Renaming the hooks to something like
usePublishmentControlto better reflect their dual nature, or- Creating separate hooks for publishing and unpublishing operations.
46-71: Add an explicit heading for better accessibility.While the modal content is clear, adding an explicit heading would improve accessibility and user experience.
<Modal size="small" visible> <ModalPanel actions={actions} onCancel={onClose} appearance={"simple"}> <Section> <Header> <WarningIcon icon="warning" /> + <h2>{t(publishItem.type === "story" ? "Unpublish Story" : "Unpublish Scene")}</h2> </Header>web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishedOrUpdatedSection.tsx (2)
9-12: Consider making props required and adding URL validation.The current props structure allows both fields to be optional, which could lead to undefined behavior. Consider:
- Making props required if they are essential for component functionality
- Adding URL format validation for
publicUrltype Props = { - isStory?: boolean; - publicUrl?: string; + isStory: boolean; + publicUrl: string; };
47-62: Enhance URL display and interaction handling.The current URL display could be improved:
- Add URL validation before opening in new tab
- Handle long URLs with truncation
- Add security attributes for external links
<UrlWrapper justify="space-between"> <UrlText hasPublicUrl - onClick={() => window.open(publicUrl, "_blank")} + onClick={() => { + if (!publicUrl || !isValidUrl(publicUrl)) return; + window.open(publicUrl, "_blank", "noopener noreferrer"); + }} + title={publicUrl} > - {publicUrl} + {truncateUrl(publicUrl, 50)} </UrlText> {/* ... */} </UrlWrapper>web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx (4)
35-51: Simplify description text logicThe current implementation uses nested ternaries and has repeated patterns. Consider refactoring for better maintainability.
const updateDescriptionText = useMemo(() => { - return publishItem.isPublished - ? publishItem.type === "story" - ? t( - `Your published story will be updated. This means all current changes will overwrite the current published story.` - ) - : t( - `Your published scene will be updated. This means all current changes will overwrite the current published scene.` - ) - : publishItem.type === "story" - ? t( - `Your story will be published. This means anybody with the below URL will be able to view this story.` - ) - : t( - `Your scene will be published. This means anybody with the below URL will be able to view this scene.` - ); + const itemType = publishItem.type; + const action = publishItem.isPublished ? "updated" : "published"; + const message = publishItem.isPublished + ? `Your ${itemType} will be ${action}. This means all current changes will overwrite the current ${action} ${itemType}.` + : `Your ${itemType} will be ${action}. This means anybody with the below URL will be able to view this ${itemType}.`; + return t(message); }, [t, publishItem]);
59-61: Extract URL click handlerConsider extracting the URL click handler to a separate function for better maintainability and testability.
+ const handleUrlClick = useCallback((url: string) => { + window.open(url, "_blank"); + }, []); + // In JSX: - <UrlWrapper onClick={() => window.open(publicUrl, "_blank")}> + <UrlWrapper onClick={() => handleUrlClick(publicUrl)}>
71-77: Improve type safety in settings navigationThe navigation logic could benefit from stronger typing to prevent potential errors.
<Button size="small" onClick={() => handleNavigationToSettings?.( "public", - publishItem.type === "story" ? publishItem.storyId : "" + publishItem.type === "story" ? publishItem.storyId : undefined ) }
1-91: Consider implementing alias uniqueness at the data layerWhile this component properly handles the UI for publishing scenes and stories, the core issue of preventing duplicate aliases should be addressed at the data layer. Consider:
- Adding a unique constraint on aliases in the database
- Implementing validation middleware for alias uniqueness
- Adding error handling for alias conflicts in this component
This will provide a more robust solution to prevent scenes and stories from sharing the same alias.
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx (1)
36-59: Rename handleProjectPublish for clarity.The function name
handleProjectPublishis misleading as it handles both stories and projects. Consider renaming it to better reflect its purpose.- const handleProjectPublish = useCallback(async () => { + const handlePublish = useCallback(async () => {Remember to update the reference in the dependencies array and the onClick handler.
web/src/services/api/storytellingApi/index.ts (2)
Line range hint
89-91: Consider using loading state to prevent concurrent operationsThe
publishStoryLoadingstate is tracked but not utilized. Consider using it to prevent concurrent publish operations on the same story.const usePublishStory = useCallback( async (s: PublishStatus, storyId?: string, alias?: string) => { if (!storyId) return; + if (publishStoryLoading) { + setNotification({ + type: "warning", + text: t("A publish operation is already in progress.") + }); + return; + }
Critical: Server-side validation exists but needs enhancement
The server has basic alias validation through
aliasRegexppattern matching and uniqueness checking viaFindByPublicName, but it doesn't enforce alias requirement for published stories. The client-side validation suggestion is still valid and would improve UX by preventing unnecessary server roundtrips.Suggested improvements:
- Client-side: Add proposed validation to require alias when publishing
- Server-side: Enhance
Publishmethod to validate alias presence for published/limited statusconst usePublishStory = useCallback( async (s: PublishStatus, storyId?: string, alias?: string) => { if (!storyId) return; + if ((s === "published" || s === "limited") && !alias) { + setNotification({ + type: "error", + text: t("Alias is required when publishing a story.") + }); + return { status: "error" }; + } const gqlStatus = toGqlStatus(s);🔗 Analysis chain
Line range hint
92-121: Critical: Add alias validation to prevent conflicts with scenesThe current implementation doesn't prevent the issue of scenes and stories sharing the same alias. The function should:
- Make the alias parameter required when publishing (status = "published" or "limited")
- Verify that the alias isn't already used by a scene before publishing
Let's verify if there's any existing alias validation:
Suggested implementation:
const usePublishStory = useCallback( async (s: PublishStatus, storyId?: string, alias?: string) => { if (!storyId) return; + if ((s === "published" || s === "limited") && !alias) { + setNotification({ + type: "error", + text: t("Alias is required when publishing a story.") + }); + return { status: "error" }; + } const gqlStatus = toGqlStatus(s); const { data, errors } = await publishStoryMutation({ variables: { storyId, alias, status: gqlStatus } });Consider adding a GraphQL validation to ensure alias uniqueness across both scenes and stories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for alias validation in the codebase echo "Searching for alias validation in GraphQL queries..." rg -A 5 "PUBLISH_STORY" echo "Searching for alias uniqueness checks..." rg -A 5 "alias.*unique|unique.*alias"Length of output: 1686
Script:
#!/bin/bash # Let's check the GraphQL schema and mutations for alias validation echo "Checking GraphQL schema for alias constraints..." rg -A 10 "type.*Story|input.*Story|mutation.*Story" echo "Checking for any existing alias validation logic..." ast-grep --pattern 'function $FUNC($$$) { $$$ if ($$$alias$$$) { $$$ } $$$ }' echo "Checking for any alias-related error messages..." rg -A 2 "alias.*error|error.*alias"Length of output: 436989
Script:
#!/bin/bash # Let's check for alias validation in the server-side implementation echo "Checking server-side alias validation..." rg -A 10 "func.*Publish.*Story" server/internal/usecase/interactor/storytelling.go echo "Checking for alias uniqueness validation..." rg -A 10 "FindByPublicName" server/internal/infrastructure/mongo/storytelling.go server/internal/infrastructure/memory/storytelling.goLength of output: 2503
Script:
#!/bin/bash # Let's check the full implementation of the Publish method echo "Checking full Publish implementation..." rg -B 5 -A 30 "func \(i \*Storytelling\) Publish" server/internal/usecase/interactor/storytelling.go echo "Checking for MatchWithPublicName implementation..." rg -B 5 -A 10 "MatchWithPublicName" server/pkg/storytelling/story.goLength of output: 1484
web/src/beta/features/Editor/index.tsx (1)
Line range hint
65-65: Address the TODO comment about DndProvider.The comment indicates a pending migration from
DndProvidertoDragAndDropContext. This technical debt should be tracked.Would you like me to create a GitHub issue to track this migration task? This would help ensure the TODO doesn't get overlooked.
web/src/beta/features/Editor/hooks/useUI.ts (4)
11-16: Well-structured type definition to prevent scene/story conflicts!The
SubProjecttype effectively prevents confusion between scenes and stories through:
- Clear type discrimination using the
typefield- Explicit relationship between projects and stories
- Strong typing that enforces proper resource identification
This design aligns well with the PR's objective of preventing scenes and stories from using the same alias.
Consider adding a validation layer that uses this type information to enforce unique aliases across different resource types.
Also applies to: 19-20
42-53: Clean up commented code and approve state management improvements.The new
activeSubProjectstate management is more robust than the previous approach. However, there's commented out code that should be removed since it's been replaced.Remove the commented out code for
currentProjectTypeandhandleProjectTypeChangeas they're no longer needed:- // const [currentProjectType, setCurrentProjectType] = - // useState<VisualizerProjectType>(tab === "story" ? "story" : "default"); - - // const handleProjectTypeChange = useCallback( - // (projectType: VisualizerProjectType) => setCurrentProjectType(projectType), - // [] - // );Also applies to: 60-66
68-98: Consider refactoring the useEffect for better maintainability.While the logic effectively manages the active subproject state, it could be more maintainable.
Consider this more concise approach:
useEffect(() => { - if (!projectId) return; switch (tab) { case "story": - if (storyId) { + if (projectId && storyId) { setActiveSubProject((prev) => prev?.id === storyId ? prev : { id: storyId, type: "story", projectId, storyId } ); } break; case "map": case "widgets": + if (projectId) { setActiveSubProject((prev) => prev?.id === projectId ? prev : { id: projectId, type: "scene", projectId } ); + } break; } }, [projectId, storyId, tab]);
187-190: Clean up commented return values.The return values have been properly updated to reflect the new state management approach.
Remove the commented out return values:
return { activeSubProject, handleActiveSubProjectChange, - // currentProjectType, - // handleProjectTypeChange, handleLayerSelectFromUI, // ...rest of the valuesweb/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.tsx (2)
Line range hint
159-176: Add error handling for publish operations.The publish operations should include error handling to gracefully handle failures:
const handleProjectPublish = useCallback( async (projectId: string) => { + try { if (projectPublished) { await usePublishProject("unpublished", projectId); } if (storiesPublished && stories?.length) { const storyPromises = stories.map((story) => usePublishStory("unpublished", story.id) ); await Promise.all(storyPromises); } + } catch (error) { + console.error('Failed to unpublish project or stories:', error); + throw error; // Re-throw to handle in the calling function + } }, [projectPublished, stories, storiesPublished, usePublishProject, usePublishStory] );
Line range hint
178-187: Handle potential race conditions in project removal.The project removal process involves multiple async operations that could fail independently:
const handleProjectRemove = useCallback( async (projectId: string) => { if (!projectId) return; - handleProjectPublish(projectId); - onProjectRemove?.(projectId); - handleProjectRemoveModal(false); + try { + await handleProjectPublish(projectId); + await onProjectRemove?.(projectId); + handleProjectRemoveModal(false); + } catch (error) { + console.error('Failed to remove project:', error); + // Consider showing an error message to the user + } }, [handleProjectRemoveModal, handleProjectPublish, onProjectRemove] );web/src/beta/features/ProjectSettings/hooks.ts (3)
6-6: Remove redundant comment.The comment
// useStorytellingFetcheris redundant as the import is already listed in the import statement above.- // useStorytellingFetcher
58-92: Consider adding error handling and reducing code duplication.The unpublish function looks solid but could benefit from these improvements:
- Add try-catch for error handling
- Extract the repeated publishmentStatus check into a helper function
Consider this refactoring:
+ const isPublished = (status: string | undefined) => { + const publishmentStatus = toPublishmentStatus(status); + return publishmentStatus === "published" || publishmentStatus === "limited"; + }; const unpublish = useCallback(async () => { + try { - const publishmentStatus = toPublishmentStatus(project?.publishmentStatus); - if (publishmentStatus === "published" || publishmentStatus === "limited") { + if (isPublished(project?.publishmentStatus)) { await usePublishProject("unpublished", projectId); } const storiesPublished = scene?.stories?.some((story) => - const publishmentStatus = toPublishmentStatus(story.publishmentStatus); - return publishmentStatus === "published" || publishmentStatus === "limited"; + isPublished(story.publishmentStatus) ); if (storiesPublished && scene?.stories) { await Promise.all( scene.stories.map(async (story) => { - const publishmentStatus = toPublishmentStatus(story.publishmentStatus); - if (publishmentStatus === "published" || publishmentStatus === "limited") { + if (isPublished(story.publishmentStatus)) { await usePublishStory("unpublished", story.id); } }) ); } + } catch (error) { + console.error("Failed to unpublish:", error); + throw error; + } }, [projectId, project?.publishmentStatus, scene?.stories, usePublishProject, usePublishStory]);
Line range hint
94-121: Consider moving setDisabled after successful unpublish.The current implementation sets the disabled state before the unpublish operation, which could lead to UI inconsistency if the unpublish fails. Consider moving it after the successful completion of both unpublish and project removal.
const handleProjectRemove = useCallback(async () => { - setDisabled(!disabled); const updatedProject = { projectId, deleted: true }; await unpublish(); const { status } = await useUpdateProjectRemove(updatedProject); client.cache.evict({ id: client.cache.identify({ __typename: "Project", id: projectId }) }); client.cache.gc(); if (status === "success") { + setDisabled(true); navigate(`/dashboard/${workspaceId}/`); } }, [client.cache, disabled, unpublish, navigate, projectId, useUpdateProjectRemove, workspaceId]);web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (1)
Line range hint
250-280: Consider adding alias conflict validation during project removal.While the code correctly handles unpublishing before removal, it might be worth adding validation to ensure no alias conflicts are left behind. This could prevent scenarios where removing a project might free up an alias that could then conflict with other scenes or stories.
Consider adding a check before removal:
const handleProjectRemove = useCallback( async (project: Project) => { const updatedProject = { projectId: project.id, deleted: true }; + // Check for potential alias conflicts before unpublishing + const aliasConflicts = await checkAliasConflicts(project.id); + if (aliasConflicts) { + throw new Error('Cannot remove project due to alias conflicts'); + } if (project?.status === "limited") { await usePublishProject("unpublished", project.id); } await useUpdateProjectRemove(updatedProject);web/src/services/i18n/translations/en.yml (1)
201-205: LGTM with a suggestion for additional context.The new translation messages clearly distinguish between scenes and stories, which aligns with the PR's goal of preventing alias conflicts. However, consider adding an additional message to explicitly warn users about alias uniqueness requirements.
Consider adding a new translation key to warn users about alias uniqueness, for example:
Your scene will be published. This means anybody with the below URL will be able to view this scene.: '' +Scene alias must be unique and cannot be the same as any existing story alias.: '' Your story will be published. This means anybody with the below URL will be able to view this story.: '' +Story alias must be unique and cannot be the same as any existing scene alias.: ''web/src/services/api/projectApi.ts (2)
Line range hint
149-151: Integrate alias check before updates.The
useProjectAliasCheckLazyQueryexists but isn't being used in the alias update flow. To prevent alias conflicts:
- Call this query before updating aliases
- Only proceed with the update if the check passes
Here's a suggested implementation:
const useUpdateProjectAlias = useCallback( async (input: UpdateProjectAliasMutationVariables) => { if (!input.projectId) return { status: "error" }; // Check alias availability first const [checkAlias] = useProjectAliasCheckLazyQuery(); const { data: checkData } = await checkAlias({ variables: { alias: input.alias, projectId: input.projectId } }); if (checkData?.checkProjectAlias?.available === false) { setNotification({ type: "error", text: t("Alias is already in use.") }); return { status: "error" }; } // Proceed with update if alias is available const { data, errors } = await updateProjectAliasMutation({ variables: { ...input } }); // ... rest of the function }, [updateProjectAliasMutation, useProjectAliasCheckLazyQuery, t, setNotification] );
Line range hint
251-297: Verify alias availability during publish.The
usePublishProjectfunction accepts an alias parameter but doesn't verify its availability before publishing. This could lead to conflicts if the same alias is used across different entities.Suggested implementation:
const usePublishProject = useCallback( async (s: PublishStatus, projectId?: string, alias?: string) => { if (!projectId) return; // If an alias is provided, verify its availability first if (alias) { const [checkAlias] = useProjectAliasCheckLazyQuery(); const { data: checkData } = await checkAlias({ variables: { alias, projectId } }); if (checkData?.checkProjectAlias?.available === false) { setNotification({ type: "error", text: t("Cannot publish: Alias is already in use.") }); return { status: "error" }; } } const gqlStatus = toGqlStatus(s); // ... rest of the function }, [publishProjectMutation, useProjectAliasCheckLazyQuery, t, setNotification] );web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts (2)
59-59: Simplify thebuttonTitlecondition for better readabilityThe condition used to assign
buttonTitlecan be simplified for clarity.Consider applying this change:
- buttonTitle: s.title !== "Default" && s.title ? s.title : t("Story"), + buttonTitle: s.title && s.title !== "Default" ? s.title : t("Story"),
60-60: Clarify the logic foraliasassignmentThe condition for setting the
aliasproperty may be confusing. Rewriting it can enhance readability and make the intention clearer.Consider restructuring the condition:
- alias: s.alias && s.alias !== project.alias ? s.alias : undefined, + alias: s.alias && s.alias === project.alias ? undefined : s.alias,web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx (1)
Line range hint
181-190: Consider using theme colors instead of hardcoded hex values for consistency.Using theme colors promotes consistency across the application and makes future maintenance easier.
Apply this diff to replace the hardcoded color with a theme color:
- backgroundColor: isPublished ? "#24A148" : theme.content.weaker, + backgroundColor: isPublished ? theme.colors.success : theme.content.weaker,web/src/beta/features/Editor/hooks/index.ts (4)
104-104: Remove commented-out codehandlePageDuplicateThe
handlePageDuplicatefunction is commented out and marked as not in use. To maintain code cleanliness and readability, consider removing this unused code.Apply this diff to remove the commented-out code:
- // handlePageDuplicate // not in use
118-119: Remove commented-out variablescurrentProjectTypeandhandleProjectTypeChangeThe variables
currentProjectTypeandhandleProjectTypeChangeare commented out. If they are no longer necessary, it's advisable to remove them to keep the codebase clean and maintainable.Apply this diff to remove the commented-out code:
- // currentProjectType, - // handleProjectTypeChange,
305-310: Remove commented-out properties inpublishPageValueWithin the
publishPageValueobject, the propertiesstoryId,selectedProjectType, andhandleProjectTypeChangeare commented out. To enhance code readability and reduce clutter, consider removing these if they are no longer needed.Apply this diff to remove the commented-out properties:
- // storyId: currentProjectType === "story" ? selectedStory?.id : undefined, ... - // selectedProjectType: currentProjectType, - // handleProjectTypeChange
313-320: Remove commented-out dependencies in the dependency arrayIn the dependency array of
publishPageValue, the variablescurrentProjectType,selectedStory?.id, andhandleProjectTypeChangeare commented out. Removing unused dependencies helps prevent confusion and potential maintenance issues.Apply this diff to clean up the dependency array:
- // currentProjectType, - // selectedStory?.id, ... - // handleProjectTypeChange
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (24)
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.tsx(1 hunks)web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts(1 hunks)web/src/beta/features/Dashboard/type.ts(2 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/hooks.ts(0 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/index.tsx(0 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishedOrUpdatedSection.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx(3 hunks)web/src/beta/features/Editor/Publish/context.tsx(1 hunks)web/src/beta/features/Editor/hooks/index.ts(4 hunks)web/src/beta/features/Editor/hooks/useUI.ts(3 hunks)web/src/beta/features/Editor/index.tsx(2 hunks)web/src/beta/features/ProjectSettings/hooks.ts(3 hunks)web/src/services/api/projectApi.ts(1 hunks)web/src/services/api/publishTypes.ts(1 hunks)web/src/services/api/storytellingApi/index.ts(1 hunks)web/src/services/api/toGqlStatus.ts(0 hunks)web/src/services/i18n/translations/en.yml(1 hunks)web/src/services/i18n/translations/ja.yml(1 hunks)
💤 Files with no reviewable changes (3)
- web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/hooks.ts
- web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/index.tsx
- web/src/services/api/toGqlStatus.ts
🔇 Additional comments (30)
web/src/services/api/publishTypes.ts (2)
1-3: LGTM! Clear and well-typed definitions.
The PublishStatus type effectively captures the three possible states and follows TypeScript best practices.
1-22: Verify connection to PR objective.
This file handles publish status types but doesn't directly address the PR's objective of preventing scenes and stories from sharing the same alias. Let's verify how this file contributes to that goal.
web/src/beta/features/Dashboard/type.ts (2)
11-11: LGTM! Verify the impact of the type change.
The change to use PublishStatus type aligns with the effort to standardize status handling across the application.
Let's verify the impact of this type change:
#!/bin/bash
# Description: Check for any potential breaking changes or places that need updates
# Expected: Find places where Project.status is used to ensure they're compatible
# Find usages of Project.status
ast-grep --pattern 'project.status'
# Find components that might need updates
rg -l "Project.*status.*published|limited|unpublished"2-2: LGTM! Verify the PublishStatus type definition.
Let's verify the imported type definition to ensure it meets our requirements:
✅ Verification successful
PublishStatus type is well-defined and correctly used
The imported PublishStatus type is properly defined in publishTypes.ts as a union type of "published" | "limited" | "unpublished", which maintains all the necessary status values. The type is also consistently used across the codebase with a proper conversion utility (toGqlStatus).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the PublishStatus type definition and its usage
# Expected: Find the type definition and verify it includes all necessary status values
# Check the type definition
rg -A 5 "type PublishStatus"
# Check other files that might need updates due to this type change
rg -l "status\?: (\"|')published(\"|') \| (\"|')limited(\"|') \| (\"|')unpublished(\"|')"
Length of output: 1666
Script:
#!/bin/bash
# Let's check the PublishStatus type definition in the specific file
rg -A 5 "PublishStatus" web/src/services/api/publishTypes.ts
# Also check for any enum or type definitions related to publish status
ast-grep --pattern 'type PublishStatus = $$$'
ast-grep --pattern 'enum PublishStatus { $$$ }'
Length of output: 551
web/src/beta/features/Editor/Publish/context.tsx (1)
4-4: Verify SubProject type implementation.
Let's verify that the SubProject type properly distinguishes between scenes and stories to prevent alias conflicts.
✅ Verification successful
SubProject type implementation is well-defined and properly distinguishes between scenes and stories
The SubProject type in web/src/beta/features/Editor/hooks/useUI.ts has a clear implementation that:
- Uses a discriminated union with
type: "scene" | "story"to distinguish between scenes and stories - Includes proper ID fields (
id,projectId) for unique identification - Has an optional
storyIdfield for scene-specific references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the SubProject type implementation
# Expected: Type definition that clearly distinguishes between scenes and stories
# Search for SubProject type definition
ast-grep --pattern 'type SubProject = $_' || ast-grep --pattern 'interface SubProject {$$$}'
# Search for usage of SubProject to verify proper type discrimination
rg -A 5 'SubProject.*{' web/src/beta/features/Editor
Length of output: 582
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx (2)
1-2: LGTM! Clean and minimal imports.
1-63: Verify alias conflict handling in parent components.
While these styled components provide the UI structure, ensure that the parent components implement proper validation to prevent scenes and stories from using the same alias.
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts (1)
1-6: Verify alias uniqueness across both scenes and stories.
Given that the PR aims to fix duplicate aliases between scenes and stories, we should ensure that useProjectAliasCheckLazyQuery validates uniqueness across both entity types.
✅ Verification successful
Based on the server implementation found, I can now generate the final response:
The alias uniqueness check is properly implemented at the server level
The server's implementation in server/internal/usecase/interactor/project.go shows that alias uniqueness is enforced through FindByPublicName, which checks for any existing project (both scenes and stories) with the given alias. The implementation:
- Validates the alias pattern
- Checks for any existing project with the same public name (alias)
- Returns availability status accordingly
This ensures that aliases remain unique across all project types, effectively preventing the duplicate alias issue between scenes and stories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the API query considers both scenes and stories
# Expected: The GraphQL query should include checks for both entity types
# Search for the GraphQL query definition
rg -l "checkProjectAlias" --type graphql
Length of output: 72
Script:
#!/bin/bash
# Let's try searching for the GraphQL query without file type restriction
rg -l "checkProjectAlias"
# Also search for any alias-related GraphQL queries or mutations
rg -l "query.*Alias|mutation.*Alias"
# Search for the actual API hook usage
ast-grep --pattern 'useProjectFetcher($$$)'
Length of output: 1850
Script:
#!/bin/bash
# Let's check the GraphQL query definition in the server and its implementation
cat server/gql/project.graphql | grep -A 10 "checkProjectAlias"
# Also check the resolver implementation
cat server/internal/adapter/gql/resolver_query.go | grep -A 20 "CheckProjectAlias"
# Check the TypeScript query definition
cat web/src/services/gql/queries/project.ts | grep -A 10 "checkProjectAlias"
Length of output: 1471
Script:
#!/bin/bash
# Let's check the loader implementation to see how alias availability is checked
rg -A 20 "CheckAlias" server/internal/
# Also check the project model/repository for alias uniqueness constraints
rg -A 10 "alias.*unique" server/
Length of output: 5052
web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx (2)
1-15: LGTM! Clean imports and well-defined types.
1-73: Verify the alias conflict prevention implementation.
While this component handles unpublishing cleanly, it's not immediately clear how it prevents scenes and stories from sharing the same alias as mentioned in the PR objective.
Let's check for the alias-related code:
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishedOrUpdatedSection.tsx (1)
1-90: Verify alias conflict prevention implementation.
While this component properly differentiates between scenes and stories in the UI, we should verify that the alias conflict prevention is properly implemented in the publishing logic.
✅ Verification successful
Let me check the alias validation implementation in the found files.
Based on the implementation shown in the files, I can now generate the final response:
Alias conflict prevention is properly implemented
The codebase has robust alias conflict prevention through multiple mechanisms:
- Stories use a different alias than scenes (in hooks.ts:
alias: s.alias && s.alias !== project.alias ? s.alias : undefined) - New aliases are validated against existing ones using
checkProjectAliasGraphQL query - Auto-generation of unique aliases with up to 3 retries if conflicts occur
- Publishing is prevented until a valid, non-conflicting alias is confirmed (
aliasValidstate)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alias-related validation logic in the codebase
echo "Searching for alias validation in publish-related files..."
rg -l 'alias' --glob 'web/src/beta/features/Editor/Publish/**/*.{ts,tsx}'
echo "Searching for publish validation logic..."
ast-grep --pattern 'function $_(publishItem) {
$$$
if ($$$alias$$$) {
$$$
}
$$$
}'
Length of output: 693
Script:
#!/bin/bash
# Check the alias validation implementation
echo "Checking useAlias.ts implementation..."
cat web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts
echo -e "\nChecking hooks.ts implementation..."
cat web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts
echo -e "\nChecking PublishOrUpdateModal/index.tsx..."
cat web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx
Length of output: 9124
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx (1)
14-14: Verify PublishItem type implementation for alias handling
The component relies on the PublishItem type from hooks. Let's verify its implementation to ensure it properly handles the distinction between scene and story aliases.
Also applies to: 16-21
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx (3)
1-19: LGTM! Clean imports and well-defined types.
The imports are properly organized and the Props interface is clearly defined with appropriate types.
113-113: Verify the alias format requirements.
The current implementation removes all forward slashes from the alias. We should verify if this is the intended behavior and if there are any format requirements for aliases.
#!/bin/bash
# Description: Check for alias format requirements and usage patterns
# Expected: Find documentation or existing code that defines alias format rules
# Search for alias-related constants or validation rules
rg -A 5 'ALIAS_PATTERN|ALIAS_REGEX|alias.*pattern'
# Search for other instances of alias manipulation
rg -A 5 'alias.*replace|formatAlias|sanitizeAlias'21-35: Verify the useAlias hook implementation for alias uniqueness.
Given the PR objective to fix the issue where "scenes and stories could use the same alias by mistake", we should verify that the useAlias hook properly validates alias uniqueness across both scenes and stories.
web/src/services/api/storytellingApi/index.ts (1)
19-19: LGTM: Import consolidation improves code organization
The consolidation of PublishStatus and toGqlStatus imports from a single source improves code organization and maintainability.
web/src/beta/features/Editor/index.tsx (1)
36-36: 🛠️ Refactor suggestion
Verify null safety in the story panel visibility check.
The refactoring from currentProjectType to activeSubProject provides better type safety and structure. However, ensure that the story panel visibility check at line 86 handles cases where activeSubProject is null/undefined.
Consider adding a null check:
-showStoryPanel={activeSubProject?.type === "story"}
+showStoryPanel={activeSubProject != null && activeSubProject.type === "story"}Let's verify the usage:
Also applies to: 86-86
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.tsx (3)
8-8: LGTM: Good standardization of publishment status handling.
The addition of toPublishmentStatus import helps maintain consistency in handling publication statuses across the application.
Line range hint 1-187: Clarify the connection between these changes and alias conflict prevention.
While the changes improve publish status management and project removal, it's not immediately clear how they prevent scenes and stories from using the same alias. Please provide additional context:
- Is this part of a larger change set?
- Are there companion changes in other files that handle the alias validation?
- How do these publish status changes contribute to preventing alias conflicts?
Line range hint 142-157: Verify the relationship between publish status and alias conflicts.
While the publish status checks are well-implemented, it's not immediately clear how these changes prevent scenes and stories from using the same alias. Please clarify:
- How does the publish status relate to alias management?
- Are there additional checks needed to prevent alias conflicts?
✅ Verification successful
Let me search for more specific validation logic around story publishing and alias conflicts.
Based on the search results, I can now provide a definitive response about the relationship between publish status and alias conflicts.
The publish status checks are correctly implemented and separate from alias conflict prevention
The codebase shows that alias conflicts are handled separately from publish status through dedicated validation:
- Server-side validation in
server/internal/usecase/interactor/storytelling.gochecks for alias conflicts before allowing publication:
if publishedStory, err := i.storytellingRepo.FindByPublicName(ctx, *inp.Alias); err != nil && !errors.Is(err, rerror.ErrNotFound) {
return nil, err
} else if publishedStory != nil && story.Id() != publishedStory.Id() {
return nil, interfaces.ErrProjectAliasAlreadyUsed
}- The publish status checks in the reviewed code (
projectPublishedandstoriesPublished) are only used to determine visibility state, not for alias validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alias-related code in the codebase
echo "Searching for alias handling in the codebase..."
rg -g '!{dist,node_modules}' -i 'alias.*story|story.*alias|alias.*scene|scene.*alias'
# Search for potential validation logic
echo "Searching for validation logic..."
ast-grep --pattern 'function $_(alias, $$$) {
$$$
}'
Length of output: 88992
Script:
#!/bin/bash
# Search for story publish validation logic
echo "Searching for story publish validation..."
rg -g '!{dist,node_modules}' -A 10 'func.*publish.*story|func.*story.*publish'
# Search for alias conflict checks
echo "Searching for alias conflict checks..."
rg -g '!{dist,node_modules}' -A 5 'FindByPublicName|UpdateAlias|MoveStory'
Length of output: 121441
web/src/beta/features/ProjectSettings/hooks.ts (1)
Line range hint 1-214: Verify implementation of alias conflict prevention.
The changes in this file focus on unpublishing logic but don't directly address the PR objective of preventing scenes and stories from using the same alias. Let's verify the implementation across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
Alias conflict prevention is properly implemented in the backend
The codebase shows proper handling of alias conflicts between scenes and stories:
- The backend has a MongoDB index to prevent duplicate aliases
- The
checkProjectAliasquery validates alias availability before updates - The frontend correctly handles alias conflicts by:
- Using
checkProjectAliasbefore updates - Ensuring story aliases don't conflict with project aliases (line 6 in PublishToolsPanel/hooks.ts:
alias: s.alias && s.alias !== project.alias ? s.alias : undefined)
- Using
The changes in the PR are consistent with maintaining alias uniqueness across the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for alias-related validation logic
# Test 1: Look for alias validation in API calls
echo "Searching for alias validation in API calls..."
rg -l "alias.*validation|validateAlias|checkAlias" --type ts
# Test 2: Look for alias-related GraphQL mutations
echo "Searching for alias-related GraphQL mutations..."
rg -l "mutation.*Alias|updateAlias" --type graphql
# Test 3: Look for recent changes to files containing alias-related code
echo "Searching for recent changes to alias-related files..."
git log -p --since="1 month ago" | rg -l "alias"
Length of output: 533
Script:
#!/bin/bash
# Let's try a different approach to find alias-related code
# Search for alias-related code in TypeScript/JavaScript files
echo "Searching for alias handling in TS/JS files..."
rg -A 5 -B 5 "updateProjectAlias|updateStoryAlias" --type ts
# Search for alias-related GraphQL operations in all file types
echo "Searching for GraphQL alias operations..."
rg -A 5 -B 5 "mutation.*updateAlias|input.*Alias"
# Search for alias validation or uniqueness checks
echo "Searching for alias validation patterns..."
rg -A 5 -B 5 "alias.*unique|alias.*exist|alias.*duplicate"
Length of output: 43188
Script:
#!/bin/bash
# Let's check the backend implementation for alias validation
# Search for alias validation in Go files
echo "Searching for alias validation in Go files..."
rg -A 10 -B 10 "func.*Publish|func.*Update.*Project|func.*Update.*Story" --type go
# Search for MongoDB index creation for alias uniqueness
echo "Searching for MongoDB index setup..."
rg -A 5 -B 5 "CreateIndex|EnsureIndex.*alias" --type go
# Search for alias validation tests
echo "Searching for alias-related tests..."
rg -A 5 -B 5 "Test.*Alias|alias.*conflict|duplicate.*alias" --type go
Length of output: 106261
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (3)
Line range hint 60-90: LGTM! Consistent usage of centralized publish status transformation.
The toPublishmentStatus function is correctly used to transform the publish status of each project.
Line range hint 1-1: Verify related files for complete alias conflict prevention.
While this file shows improvements in publish status handling, we should verify the complete implementation of alias conflict prevention.
Let's check for related files:
#!/bin/bash
# Description: Find related files that handle scene/story aliases
# Test 1: Look for alias-related code
rg -l "alias.*conflict|scene.*alias|story.*alias" "src/"
# Test 2: Look for publish-related types and validations
rg -l "PublishStatus|publishmentStatus" "src/services/"5-5: LGTM! Centralized publish status handling is a good practice.
Moving toPublishmentStatus to a centralized location helps ensure consistent handling of publish statuses across scenes and stories.
Let's verify the implementation of the centralized function:
✅ Verification successful
Based on the evidence found, I can now generate the final response:
Verified: The centralized toPublishmentStatus is part of a robust alias validation system
The server-side implementation in server/internal/usecase/interactor/storytelling.go shows that alias conflicts between scenes and stories are prevented through:
- Validation before publishing by checking if the alias is already in use
- Returning
ErrProjectAliasAlreadyUsederror if the alias is taken by another story - Proper alias update handling through
UpdateAliasmethod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of toPublishmentStatus to ensure it handles alias conflicts
# Test: Look for alias validation in the implementation
rg -A 10 "toPublishmentStatus.*=.*" "src/services/api/publishTypes"
Length of output: 211
Script:
#!/bin/bash
# Let's try searching from the repository root with correct paths
# First, find the publishTypes file
fd "publishTypes" --type f
# Then check its content for toPublishmentStatus
rg -A 10 "toPublishmentStatus.*=.*"
# Also search for any alias-related validation
rg -A 5 "alias.*conflict|scene.*alias|story.*alias"
Length of output: 68074
Script:
#!/bin/bash
# Let's check the server-side implementation for alias validation
# First, find the relevant GraphQL resolver
fd "publishStory.*resolver" server/
# Then check for any alias validation in the storytelling package
rg -A 5 "func.*publish.*story|alias.*validation|check.*alias" server/internal/usecase/storytelling/
Length of output: 217
Script:
#!/bin/bash
# Let's try searching in the server directory for story publishing and alias validation
# First, find all files related to story publishing
fd "story" server/
# Then check for alias validation in those files
rg -A 5 "publish.*story|alias.*validation|check.*alias|unique.*alias" server/
Length of output: 4748
web/src/services/api/projectApi.ts (2)
41-42: LGTM: Import consolidation looks good.
The consolidation of publish-related types and utilities into a single publishTypes module improves code organization.
Line range hint 393-417: Add validation to prevent alias conflicts.
The useUpdateProjectAlias function doesn't validate alias uniqueness before updating, which could lead to conflicts between scenes and stories. Consider using the existing useProjectAliasCheckLazyQuery before performing the update.
Let's verify the current alias validation across the codebase:
web/src/services/i18n/translations/ja.yml (1)
202-205: LGTM! Clear and consistent translations.
The translations for story/scene publishing and updating messages are:
- Accurate in conveying the meaning
- Consistent in terminology between story and scene versions
- Properly formatted with correct Japanese punctuation
- Clear in explaining the implications of publishing/updating
web/src/beta/features/Editor/hooks/index.ts (3)
99-113: Integration of storytelling functionality looks good
The addition of storytelling-related variables from useStorytelling enhances the editor's capability to handle storytelling features effectively. The implementation is appropriate and aligns with the new focus on storytelling within the editor context.
Line range hint 115-136: Updates to useUI hook usage are appropriate
Incorporating activeSubProject and handleActiveSubProjectChange from the useUI hook aligns with the updated publish management logic. Passing storyId: selectedStory?.id as a parameter ensures that the UI correctly reflects the selected story context.
334-334: Addition of activeSubProject to return object is appropriate
Including activeSubProject in the returned object ensures that the active sub-project information is accessible where needed. This change supports the updated publish management functionality.
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx
Outdated
Show resolved
Hide resolved
...features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishedOrUpdatedSection.tsx
Outdated
Show resolved
Hide resolved
...ta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (35)
web/src/beta/features/Dashboard/type.ts (1)
11-11: Type safety improvement for project status.The change from a hardcoded union type to
PublishStatusimproves type safety and maintainability. This is particularly important as it relates to the PR's objective of preventing alias conflicts between scenes and stories.Consider documenting the valid status transitions in the codebase to prevent invalid state changes. This could be done through:
- TypeScript type guards
- A state machine pattern
- Comprehensive unit tests for status transitions
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx (3)
4-13: Consider enhancing accessibility.The Section component handles visual feedback well, but could benefit from ARIA attributes to improve accessibility.
Consider adding aria-disabled attribute:
export const Section = styled("div")<{ disabled?: boolean }>( ({ disabled, theme }) => ({ display: "flex", flexDirection: "column", padding: theme.spacing.normal, gap: theme.spacing.large, opacity: disabled ? 0.6 : 1, - cursor: disabled ? "not-allowed" : "auto" + cursor: disabled ? "not-allowed" : "auto", + "&[aria-disabled='true']": { + opacity: 0.6, + cursor: "not-allowed" + } }) );
21-23: Consider using Typography props directly.This component only sets text alignment. Consider using Typography with textAlign prop directly at usage sites unless this component is used in multiple places.
25-28: Consider using theme-based dimensions.Hard-coded dimensions might limit flexibility. Consider using theme spacing units for consistency.
-export const WarningIcon = styled(Icon)({ - width: "24px", - height: "24px" -}); +export const WarningIcon = styled(Icon)(({ theme }) => ({ + width: theme.spacing.large * 1.5, + height: theme.spacing.large * 1.5 +}));web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts (4)
1-6: Add return type interface for better type safety.Consider defining an interface for the hook's return value to improve type safety and documentation.
+interface UseAliasResult { + alias: string | undefined; + aliasValid: boolean; +} export default ({ publishItem }: { publishItem: PublishItem }) => {
56-60: Consider returning additional metadata.The hook could provide more useful information to the consumer, such as loading state and error information.
return { alias, - aliasValid + aliasValid, + isLoading: validatingAlias, + hasError: createAliasCount.current > 3 };
62-64: Enhance alias generation flexibility.Consider making the alias length configurable and adding character set restrictions to ensure valid URLs.
-function generateAlias() { - return generateRandomString(10); +function generateAlias(length: number = 10) { + // Ensure URL-safe characters only + return generateRandomString(length).replace(/[^a-zA-Z0-9-]/g, ''); }
1-64: Address core PR objective: Preventing scene/story alias conflicts.While this implementation manages aliases, it doesn't fully prevent scenes and stories from using the same alias. To address the PR's main objective, consider:
- Prefixing aliases with the content type (e.g., "scene-" or "story-")
- Adding validation to check against both scene and story aliases
- Implementing a more robust uniqueness check
Consider moving the alias validation logic to a separate service that maintains a registry of all aliases across different content types.
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishedOrUpdatedSection.tsx (1)
18-22: Improve embed code generation.The current implementation has two issues:
- Hardcoded iframe dimensions (560x315) might not suit all use cases
- There's an unnecessary semicolon at the end of the iframe code
Consider this improvement:
const embedCode = useMemo( () => - `<iframe width="560" height="315" src="${publicUrl}" frameBorder="0"></iframe>;`, + `<iframe width="100%" height="600" src="${publicUrl}" frameBorder="0" style="max-width: 100%;"></iframe>`, [publicUrl] );web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx (2)
35-51: Simplify conditional text logicThe nested ternary operations make the code harder to read and maintain.
Consider this more maintainable approach:
const updateDescriptionText = useMemo(() => { - return publishItem.isPublished - ? publishItem.type === "story" - ? t( - `Your published story will be updated. This means all current changes will overwrite the current published story.` - ) - : t( - `Your published scene will be updated. This means all current changes will overwrite the current published scene.` - ) - : publishItem.type === "story" - ? t( - `Your story will be published. This means anybody with the below URL will be able to view this story.` - ) - : t( - `Your scene will be published. This means anybody with the below URL will be able to view this scene.` - ); + const itemType = publishItem.type; + const messageKey = `${publishItem.isPublished ? 'update' : 'publish'}.${itemType}`; + const messages = { + 'update.story': t('Your published story will be updated. This means all current changes will overwrite the current published story.'), + 'update.scene': t('Your published scene will be updated. This means all current changes will overwrite the current published scene.'), + 'publish.story': t('Your story will be published. This means anybody with the below URL will be able to view this story.'), + 'publish.scene': t('Your scene will be published. This means anybody with the below URL will be able to view this scene.') + }; + return messages[messageKey]; }, [t, publishItem]);
81-86: Improve search engine indexing switch handlingThe switch should be disabled if no handler is provided to prevent user confusion.
<SwitchField title={t("Search engine indexing")} description={t("Page will be available as result on search engines")} value={searchEngineIndexEnabled} + disabled={!handleSearchIndexEnableChange} onChange={handleSearchIndexEnableChange} />web/src/beta/features/Editor/index.tsx (1)
36-36: Improved type safety withactiveSubProjectThe rename from
currentProjectTypetoactiveSubProjectis a good improvement as it provides access to the full sub-project object rather than just its type. This change enables better type checking and access to additional properties that could help prevent alias conflicts between scenes and stories.web/src/beta/features/Editor/hooks/useUI.ts (3)
60-66: Remove commented-out code.The old implementation using
currentProjectTypeshould be removed rather than commented out, as it's being replaced by the newactiveSubProjectstate.- // const [currentProjectType, setCurrentProjectType] = - // useState<VisualizerProjectType>(tab === "story" ? "story" : "default"); - - // const handleProjectTypeChange = useCallback( - // (projectType: VisualizerProjectType) => setCurrentProjectType(projectType), - // [] - // );
42-53: Consider enhancing state initialization.The initial state setup could be more robust by considering both
projectIdandstoryIdduring initialization, similar to how they're handled in the effect.const [activeSubProject, setActiveSubProject] = useState<SubProject | undefined>( - projectId - ? { - id: projectId, - type: "scene", - projectId - } - : undefined + projectId + ? storyId + ? { + id: storyId, + type: "story", + projectId, + storyId, + } + : { + id: projectId, + type: "scene", + projectId, + } + : undefined );
187-190: Clean up return object.Remove the commented-out properties from the return object to maintain clean code.
return { activeSubProject, handleActiveSubProjectChange, - // currentProjectType, - // handleProjectTypeChange, handleLayerSelectFromUI, // ...rest of the propertiesweb/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.tsx (1)
Line range hint
206-213: Consider adding error handling.The project removal process could benefit from error handling to gracefully handle failures during the publish cleanup or removal process.
Consider wrapping the operations in a try-catch block:
const handleProjectRemove = useCallback( async (projectId: string) => { if (!projectId) return; - handleProjectPublish(projectId); - onProjectRemove?.(projectId); - handleProjectRemoveModal(false); + try { + await handleProjectPublish(projectId); + await onProjectRemove?.(projectId); + handleProjectRemoveModal(false); + } catch (error) { + console.error('Failed to remove project:', error); + // Consider adding error notification here + } }, [handleProjectRemoveModal, handleProjectPublish, onProjectRemove] );web/src/beta/features/ProjectSettings/hooks.ts (2)
6-6: Remove unnecessary comment.The commented import
// useStorytellingFetcheris redundant as the actual import is already included in the import list.- // useStorytellingFetcher
Let me analyze the search results for
setDisabledusage patterns:
- In
RecycleBin/hooks.ts, there's a proper error handling pattern:setDisabled(true); try { // async operations } catch { setDisabled(false); }
In
PasswordModal/index.tsx, it's used for form validation state.In the code under review (
ProjectSettings/hooks.ts), the disabled state is toggled without proper error handling or cleanup.Based on this evidence, I can generate the final response:
Add proper error handling and state management for the disabled state.
The current implementation toggles the disabled state without handling errors or ensuring it's reset on failure. This can leave the UI in a permanently disabled state if an error occurs. Follow the established pattern from the codebase:
const handleProjectRemove = useCallback(async () => { - const updatedProject = { - projectId, - deleted: true - }; - setDisabled(!disabled); + try { + setDisabled(true); + const updatedProject = { + projectId, + deleted: true + }; - await unpublish(); + await unpublish(); + const { status } = await useUpdateProjectRemove(updatedProject); + client.cache.evict({ + id: client.cache.identify({ + __typename: "Project", + id: projectId + }) + }); + client.cache.gc(); + if (status === "success") { + navigate(`/dashboard/${workspaceId}/`); + } + } catch (error) { + setDisabled(false); + throw error; + } }, [ client.cache, disabled, unpublish, navigate, projectId, useUpdateProjectRemove, workspaceId ]);🔗 Analysis chain
Line range hint
94-117: Review the disabled state management.The function toggles the disabled state at the beginning but doesn't handle re-enabling on failure or completion. Consider these improvements:
- Move the disable state toggle after successful unpublish
- Add error handling to re-enable on failure
const handleProjectRemove = useCallback(async () => { - const updatedProject = { - projectId, - deleted: true - }; - setDisabled(!disabled); + try { + setDisabled(true); + const updatedProject = { + projectId, + deleted: true + }; await unpublish(); const { status } = await useUpdateProjectRemove(updatedProject); client.cache.evict({ id: client.cache.identify({ __typename: "Project", id: projectId }) }); client.cache.gc(); if (status === "success") { navigate(`/dashboard/${workspaceId}/`); } + } catch (error) { + console.error('Failed to remove project:', error); + setDisabled(false); + throw error; + } }, [ client.cache, disabled, unpublish, navigate, projectId, useUpdateProjectRemove, workspaceId ]);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of setDisabled to understand the state management pattern rg -A 5 "setDisabled"Length of output: 111860
web/src/services/i18n/translations/en.yml (1)
201-202: Consider adding validation hints to embed code messages.To prevent potential issues with embedding, consider adding hints about validation or requirements for the embed code.
-'* Please use this code if you want to embed your scene into a webpage': '' +'* Please use this code if you want to embed your scene into a webpage. Ensure the target page supports iframe embedding': ''web/src/services/api/projectApi.ts (3)
Line range hint
200-250: Add validation to prevent duplicate aliases between scenes and stories.The
usePublishProjecthook accepts an alias parameter without validation against existing scene/story aliases. This could lead to the issue mentioned in the PR where scenes and stories could use the same alias.Consider adding alias validation before publishing:
- Query existing scene and story aliases
- Validate that the new alias doesn't conflict
- Proceed with publishing only if validation passes
Would you like me to help implement this validation logic?
Line range hint
400-450: Utilize existing alias check functionality.The
useProjectAliasCheckLazyQueryhook exists but isn't being used inuseUpdateProjectAlias. This could help prevent alias conflicts.Consider this implementation:
const useUpdateProjectAlias = useCallback( async (input: UpdateProjectAliasMutationVariables) => { if (!input.projectId) return { status: "error" }; + + // Check if alias is already in use + const [checkAlias] = useProjectAliasCheckLazyQuery(); + const { data: aliasData } = await checkAlias({ + variables: { alias: input.alias } + }); + + if (aliasData?.checkProjectAlias?.available === false) { + setNotification({ + type: "error", + text: t("This alias is already in use.") + }); + return { status: "error" }; + } + const { data, errors } = await updateProjectAliasMutation({ variables: { ...input } });
Line range hint
1-450: Overall architectural suggestion for preventing alias conflicts.To fully address the issue of scenes and stories using the same alias, consider implementing a comprehensive solution:
- Create a shared alias validation service
- Use the existing
useProjectAliasCheckLazyQueryconsistently across all alias-related operations- Extend the alias check to validate against both scene and story aliases
- Add appropriate error handling and user feedback
Would you like me to help design this architectural solution in more detail?
web/src/services/i18n/translations/ja.yml (1)
201-205: LGTM! Minor suggestion for line 201.The translations are clear, natural, and maintain consistent terminology. However, line 201 has an empty translation for the scene embedding code explanation.
Consider adding this translation to maintain consistency with the story embedding explanation:
-'* Please use this code if you want to embed your scene into a webpage': '' +'* Please use this code if you want to embed your scene into a webpage': 'その他のWebサイトでこのシーンを埋め込む場合は、こちらのコードを使用してください'web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts (1)
59-60: Clarify Logic forbuttonTitleandaliasPropertiesThe conditional assignment for
buttonTitleandaliasmay benefit from additional clarity or comments to ensure future maintainability. While the current logic is correct, it's slightly complex.Consider adding comments or simplifying the conditions for better readability.
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx (2)
61-71: Refactor nested ternary operators intitlecomputation for clarityThe nested ternary operators used in the
titlecomputation make the code difficult to read and maintain. Refactoring this logic can improve readability and reduce potential errors.Apply this diff to refactor the
titlecalculation:const title = useMemo(() => { - return publishDone - ? t("Congratulations!") - : !publishItem.isPublished - ? publishItem.type === "story" - ? t(`Publish your story`) - : t(`Publish your scene`) - : publishItem.type === "story" - ? t(`Update your story`) - : t(`Update your scene`); + if (publishDone) { + return t("Congratulations!"); + } + if (!publishItem.isPublished) { + return publishItem.type === "story" + ? t("Publish your story") + : t("Publish your scene"); + } + return publishItem.type === "story" + ? t("Update your story") + : t("Update your scene"); }, [t, publishDone, publishItem]);
85-90: Verify button appearance logic for user experience consistencyIn the
actionsrendering, the secondary button changes itsappearanceprop based onpublishDone. WhenpublishDoneistrue, the secondary button has an appearance of"primary", which might confuse users, as the primary action is typically highlighted. Ensure this behavior aligns with the intended user experience.web/src/beta/features/Editor/hooks/index.ts (3)
104-104: Remove unusedhandlePageDuplicatecodeThe
handlePageDuplicatefunction is commented out and marked as "not in use". To maintain code cleanliness, consider removing this commented-out code if it's no longer needed.Apply this diff to remove the unused code:
- // handlePageDuplicate // not in use
118-119: Clean up commented-out code inuseUIdestructuringThe variables
currentProjectTypeandhandleProjectTypeChangeare commented out. If these are no longer required, it's better to remove them to improve code readability.Apply this diff to remove the commented-out variables:
- // currentProjectType, - // handleProjectTypeChange,
307-307: Remove obsolete commented-out code inpublishPageValueThe commented-out code related to
currentProjectTypeandstoryIdappears to be obsolete due to recent changes. Consider removing these lines to keep the codebase clean.Apply this diff to remove the commented-out code:
- // storyId: currentProjectType === "story" ? selectedStory?.id : undefined, ... - // selectedProjectType: currentProjectType, - // handleProjectTypeChangeAlso applies to: 309-310
web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx (6)
11-11: Remove commented-out import statementThe import statement for
PublishModalis commented out. If this component is no longer needed, consider removing the commented line to keep the codebase clean and maintainable.
17-21: Clean up commented-out variablesVariables such as
storyId,selectedProjectType, andhandleProjectTypeChangeare commented out. If these are no longer in use due to recent changes, removing them will enhance code readability and reduce confusion.
29-49: Remove unused hooks and variablesSeveral hooks and variables (e.g.,
publishmentStatuses,publishing,publishStatus, etc.) are commented out. If they are obsolete, it's better to remove them entirely to maintain a clean codebase.
56-66: Eliminate deprecated status logicThe logic for determining
sceneStatusandstoryStatusis commented out. If this code is deprecated due to the new implementation, consider removing it to avoid potential confusion and to streamline the code.
125-137: Remove deprecatedPublishModalcomponentThe old
PublishModalcomponent and its related props are commented out. If this component is replaced by the new modals and no longer needed, consider removing this code to reduce clutter.
181-185: Optimize inline stylingIn the
PublishStatusstyled component, the color#24A148is hard-coded for the published status. Consider using a theme variable or a constant for this color to maintain consistency and ease future changes.Apply this diff to use a theme variable:
- backgroundColor: isPublished ? "#24A148" : theme.content.weaker, + backgroundColor: isPublished ? theme.colors.success : theme.content.weaker,Ensure that
theme.colors.successis defined in your theme.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (24)
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.tsx(1 hunks)web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts(1 hunks)web/src/beta/features/Dashboard/type.ts(2 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/hooks.ts(0 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/index.tsx(0 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishedOrUpdatedSection.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx(3 hunks)web/src/beta/features/Editor/Publish/context.tsx(1 hunks)web/src/beta/features/Editor/hooks/index.ts(4 hunks)web/src/beta/features/Editor/hooks/useUI.ts(3 hunks)web/src/beta/features/Editor/index.tsx(2 hunks)web/src/beta/features/ProjectSettings/hooks.ts(3 hunks)web/src/services/api/projectApi.ts(1 hunks)web/src/services/api/publishTypes.ts(1 hunks)web/src/services/api/storytellingApi/index.ts(1 hunks)web/src/services/api/toGqlStatus.ts(0 hunks)web/src/services/i18n/translations/en.yml(1 hunks)web/src/services/i18n/translations/ja.yml(1 hunks)
💤 Files with no reviewable changes (3)
- web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/hooks.ts
- web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/index.tsx
- web/src/services/api/toGqlStatus.ts
🔇 Additional comments (42)
web/src/services/api/publishTypes.ts (3)
1-3: LGTM! Clear and well-defined type definition.
The PublishStatus type is well-defined with descriptive values that clearly represent the possible states.
13-22: LGTM! Clean and well-structured implementation.
The function uses a clear switch statement with proper handling of all cases, including a default fallback.
1-22: Verify the connection to alias conflict resolution.
While the code is well-implemented, it's not immediately clear how these publish status changes relate to fixing the issue where "scenes and stories could use the same alias by mistake". Could you clarify the connection?
Let's check for alias-related code:
✅ Verification successful
Let me search for more specific code around alias validation and conflicts:
Based on the code analysis, I can now provide a final response:
The publish status changes are directly related to alias conflict prevention.
The code shows a robust system for preventing alias conflicts between scenes and stories:
- Server-side validation in
FindByPublicNamechecks for existing aliases across both projects and stories - When publishing, the system verifies that the alias is not already used by another project or story
- The publish status (public/limited/private) is tied to alias validation, as only public/limited items can have aliases
- The frontend uses
checkProjectAliasto validate aliases before allowing publication
This implementation ensures that scenes and stories cannot use the same alias, preventing the conflict mentioned in the PR objective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alias-related code and its connection to publish status
rg -g '!*.{lock,json,md}' -i 'alias.*publish|publish.*alias'
Length of output: 101474
Script:
#!/bin/bash
# Search for alias validation and conflict prevention code
rg -A 10 'FindByPublicName|FindByAlias|checkProjectAlias|checkStoryAlias'
Length of output: 61306
web/src/beta/features/Dashboard/type.ts (1)
2-2: LGTM! Good practice using a centralized type.
The addition of the PublishStatus import from a centralized location promotes better type consistency across the application.
Let's verify the PublishStatus type definition and its usage:
✅ Verification successful
Verified: The PublishStatus type is correctly defined and consistently used
The PublishStatus type is properly defined in web/src/services/api/publishTypes.ts as a union type of "published" | "limited" | "unpublished". The import and usage in Dashboard/type.ts is correct and aligns with the type's definition. The type is also consistently used across the codebase in relevant components and API services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the PublishStatus type definition and its usage
# Expected: Find the type definition and other files using it
# Find the PublishStatus type definition
echo "=== PublishStatus Definition ==="
rg -t typescript "export.*PublishStatus"
# Find other files using PublishStatus
echo -e "\n=== PublishStatus Usage ==="
rg -t typescript "PublishStatus" --glob '!*/publishTypes.ts'
Length of output: 319
Script:
#!/bin/bash
# Let's try without typescript file type flag since it's not recognized
# Find the PublishStatus type definition
echo "=== PublishStatus Definition ==="
rg "export.*PublishStatus"
# Find other files using PublishStatus
echo -e "\n=== PublishStatus Usage ==="
rg "PublishStatus" --glob '!*/publishTypes.ts'
# Also search for the exact file path
echo -e "\n=== File Content ==="
find . -name "publishTypes.ts" -exec cat {} \;
Length of output: 2781
web/src/beta/features/Editor/Publish/context.tsx (2)
9-10: LGTM! Type-safe SubProject management.
The new properties provide a type-safe way to manage sub-projects, which should help prevent alias conflicts between scenes and stories.
4-6: Verify complete removal of ProjectType.
The commented-out import suggests ProjectType is being phased out. Let's verify there are no remaining references.
✅ Verification successful
ProjectType has been properly replaced with SubProject
The commented-out ProjectType import and its related code have been correctly removed as part of an architectural change. The codebase now consistently uses SubProject from useUI hooks instead, with proper type definitions and usage across the Publish feature files. No active references to ProjectType remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to ProjectType
# Expect: No results if completely removed
# Search for ProjectType references
rg "ProjectType" --type ts --type tsx
# Search for specific usage patterns
ast-grep --pattern 'type ProjectType = $$$'
Length of output: 218
Script:
#!/bin/bash
# Let's try a more comprehensive search for ProjectType
rg "ProjectType" -t typescript -t tsx
# Also search in the specific directory
rg "ProjectType" web/src/beta/features/Editor/Publish/
# Check for any type definitions containing Project
ast-grep --pattern 'type Project$_ = $$$'
Length of output: 665
Script:
#!/bin/bash
# Let's check the PublishToolsPanel/hooks.ts file where ProjectType was originally imported from
rg -A 5 "ProjectType" web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts
# Also check for any active (non-commented) usage of ProjectType in these files
rg "ProjectType" web/src/beta/features/Editor/Publish/PublishToolsPanel/ --no-comments
# Check if SubProject is now being used instead
rg "SubProject" web/src/beta/features/Editor/Publish/ -A 2
Length of output: 3951
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx (4)
1-3: LGTM! Clean and well-organized imports.
15-19: LGTM! Clean implementation with proper theme usage.
30-34: LGTM! Clean implementation with proper theme usage.
61-63: LGTM! Clean implementation with proper theme usage.
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts (1)
7-17: Consider handling concurrent alias checks.
The lazy query setup could lead to race conditions if multiple checks are triggered in quick succession. Consider adding a cleanup mechanism or cancellation token.
Let's check if there are similar patterns in the codebase:
web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx (2)
1-73: Verify integration with alias conflict prevention
While this component handles unpublishing well, we should verify how it integrates with the solution for preventing scene and story alias conflicts.
#!/bin/bash
# Search for alias-related validations in the publishing workflow
rg -A 5 "alias.*validation|validateAlias" web/src/beta/features/Editor/Publish/
# Search for alias-related types or interfaces
ast-grep --pattern 'interface $_ {
$$$
alias
$$$
}'1-15: Verify PublishItem type implementation
Given the PR objective to fix alias conflicts between scenes and stories, we should ensure the PublishItem type properly distinguishes between these types.
✅ Verification successful
PublishItem type correctly handles scene/story distinction
The PublishItem type implementation properly distinguishes between scenes and stories through:
- A discriminating
typefield with literal union type"scene" | "story" - Distinct handling of aliases where stories check for conflicts with scene aliases
- Proper type fields for both scene and story identification (
projectId,storyId)
The implementation aligns well with the PR objective of fixing alias conflicts between scenes and stories, as seen in the story mapping logic where it explicitly handles alias conflicts:
alias: s.alias && s.alias !== project.alias ? s.alias : undefined, // correct existing alias, needs republish🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the PublishItem type implementation
rg -A 10 "type PublishItem" web/src/beta/features/Editor/Publish/
Length of output: 141
Script:
#!/bin/bash
# Let's try finding the hooks file that defines PublishItem
fd hooks.ts web/src/beta/features/Editor/Publish/ --exec cat {}
# Also search for any type definitions containing "PublishItem"
ast-grep --pattern 'type PublishItem = {
$$$
}'
# And interface definitions
ast-grep --pattern 'interface PublishItem {
$$$
}'
Length of output: 4072
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx (2)
31-33: LGTM!
The settings navigation hook is properly implemented with the project ID.
14-21: Verify PublishItem type implementation for alias handling
Given the PR objective to fix alias conflicts between scenes and stories, we should verify the implementation of the PublishItem type.
✅ Verification successful
Based on the results from our previous searches, I can now provide a final response:
PublishItem type properly handles scene/story distinction for alias management
The PublishItem type includes essential fields to differentiate between scenes and stories through:
type: "scene" | "story"field to explicitly distinguish the item typestoryId?: stringoptional field for scene-story relationshipalias: string | undefinedfield with proper typing
The server-side validation shows:
- Proper alias pattern validation (
aliasRegexp = regexp.MustCompile("^[a-zA-Z0-9_5-32}$")) - Distinct error handling for alias conflicts (
ErrInvalidAlias,ErrProjectAliasAlreadyUsed) - Separate validation logic for stories and projects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PublishItem type definition to verify alias handling
ast-grep --pattern 'type PublishItem = {
$$$
}'
# Search for any alias-related validation logic
rg -A 5 'alias.*conflict|duplicate.*alias|same.*alias'
Length of output: 1008
Script:
#!/bin/bash
# Let's search for validation logic related to alias in the hooks file and its usage
rg -A 10 "alias.*validation|validateAlias|checkAlias" web/src/beta/features/Editor/Publish/PublishToolsPanel/
# Search for any API calls or mutations related to alias validation
ast-grep --pattern 'mutation($$$alias$$$)'
# Search for error handling related to alias
rg -A 5 "alias.*error|error.*alias"
# Look for any functions that handle alias-related operations
ast-grep --pattern 'function $_($$$ alias $$$) {
$$$
}'
Length of output: 7383
Script:
#!/bin/bash
# Let's check for GraphQL mutations and queries related to publishing in the frontend
rg -A 10 "mutation.*Publish|query.*Publish" web/src/beta/features/Editor/Publish/
# Look for the actual implementation of publish/update logic
ast-grep --pattern 'const $_ = async ($$$) => {
$$$publish$$$
}'
# Check for any alias validation in the frontend
rg -A 10 "validateAlias|isValidAlias|checkAlias" web/src/beta/features/Editor/Publish/
Length of output: 241
web/src/services/api/storytellingApi/index.ts (2)
19-19: LGTM: Improved type imports
The consolidation of PublishStatus and toGqlStatus imports from a central publishTypes module improves code organization and type consistency.
Line range hint 93-121: Add validation to prevent alias conflicts between scenes and stories
The usePublishStory function accepts an alias parameter but doesn't validate if the same alias is already in use by a scene. This could lead to the issue mentioned in the PR objective where scenes and stories could use the same alias by mistake.
Let's verify if there's any existing validation in the GraphQL schema or other parts of the codebase:
Consider adding validation before making the mutation:
const usePublishStory = useCallback(
async (s: PublishStatus, storyId?: string, alias?: string) => {
if (!storyId) return;
// Add validation to check for alias conflicts
if (alias) {
const { data: sceneData } = await checkSceneAlias({ variables: { alias } });
if (sceneData?.scene) {
setNotification({
type: "error",
text: t("This alias is already in use by a scene. Please choose a different alias.")
});
return { status: "error" };
}
}
const gqlStatus = toGqlStatus(s);
// ... rest of the function
},
[publishStoryMutation, t, setNotification]
);web/src/beta/features/Editor/index.tsx (1)
86-86: Enhanced null safety with optional chaining
The updated condition activeSubProject?.type === "story" adds proper null checking through optional chaining. This is more defensive than the previous implementation and prevents potential runtime errors when activeSubProject is undefined.
Let's verify the handling of undefined cases:
✅ Verification successful
Optional chaining is correctly applied for null safety
The optional chaining in activeSubProject?.type === "story" is correctly implemented. The context shows that activeSubProject is passed down as a prop and used in a component where null checking is important. The optional chaining operator prevents runtime errors when activeSubProject is undefined or null, making this a proper defensive programming practice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of activeSubProject to ensure consistent null checking
rg "activeSubProject\." --type ts --type tsx
Length of output: 74
Script:
#!/bin/bash
# Search for usages of activeSubProject with proper file type
rg "activeSubProject\." "*.ts" "*.tsx"
# Also search without the dot to catch potential undefined checks
rg "activeSubProject" "*.ts" "*.tsx"
Length of output: 259
Script:
#!/bin/bash
# First, find all TypeScript/React files
fd "\.(ts|tsx)$"
# Then search for activeSubProject usages
rg "activeSubProject" --type typescript
Length of output: 43418
Script:
#!/bin/bash
# Search for activeSubProject in the specific file and its surrounding context
rg -A 5 -B 5 "activeSubProject" web/src/beta/features/Editor/index.tsx
Length of output: 873
web/src/beta/features/Editor/hooks/useUI.ts (3)
11-16: Well-structured type definitions!
The SubProject type effectively uses a discriminated union pattern with the type field, making it type-safe to distinguish between scenes and stories. The optional storyId field appropriately handles the story-specific case.
Also applies to: 19-20
11-16: Verify alias conflict prevention.
While the SubProject type helps distinguish between scenes and stories, we should verify that alias conflicts are properly prevented at the data validation level.
#!/bin/bash
# Search for alias validation logic
echo "Searching for alias validation in the codebase..."
rg -A 5 "alias.*validation|validateAlias"
# Search for alias-related mutations or API calls
echo "Searching for alias-related mutations..."
rg -A 5 "mutation.*alias|createScene|createStory"68-98: Verify tab switching behavior.
The effect handles tab switching, but there might be an edge case when switching from "story" to "map/widgets" tab while a story is active. Ensure that the story-related state is properly cleared when switching to the scene view.
✅ Verification successful
Let me search for more specific information about the tab switching and state management logic.
Let me search for the SubProject type definition to understand the state structure better.
Based on the code analysis, I can now provide the final response:
The tab switching behavior is correctly implemented.
The effect properly manages the activeSubProject state when switching between tabs:
- For "story" tab: Sets type to "story" with both projectId and storyId
- For "map"/"widgets" tabs: Sets type to "scene" with only projectId
- The state update logic uses functional updates with proper prev state checks
- When switching from story to map/widgets, the story-specific state (storyId) is automatically cleared since it's not included in the scene type object
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tab switching logic in the codebase to verify proper cleanup
rg -A 5 "handleTabChange|onTabChange"
Length of output: 97445
Script:
#!/bin/bash
# Search for tab-related state management and cleanup logic
rg -A 5 "setActiveSubProject|activeSubProject"
Length of output: 8334
Script:
#!/bin/bash
# Search for SubProject type definition and related interfaces
rg -A 5 "type SubProject"
Length of output: 403
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.tsx (5)
8-8: LGTM: Good centralization of publish status logic.
Moving toPublishmentStatus to a centralized location improves maintainability and ensures consistent status handling across the application.
Line range hint 171-173: LGTM: Clear and consistent publish status check.
The publish status check properly handles both "published" and "limited" states, making the code's behavior explicit and maintainable.
Line range hint 175-183: LGTM: Consistent story publish status handling.
The implementation correctly uses the centralized toPublishmentStatus function and maintains consistency with project status handling. The memoization is a good performance optimization.
Line range hint 190-204: LGTM: Efficient publish status cleanup.
The implementation efficiently handles unpublishing of both project and stories:
- Correctly checks current publish status
- Uses Promise.all for parallel story unpublishing
- Maintains consistency between project and story states
Line range hint 1-249: Verify complete implementation of alias conflict prevention.
While these changes improve publish status handling, please verify that additional changes in other files directly address the core issue of preventing scenes and stories from using the same alias.
✅ Verification successful
Based on the search results, I can now provide a final response:
Alias conflict prevention is properly implemented through validation checks
The codebase shows a robust implementation of alias conflict prevention:
- Projects have a
checkProjectAliasquery that validates alias availability - Stories use the same publish mutation structure with alias validation
- The
useAliashook handles alias validation and auto-generation when conflicts occur - Both project and story publishing flows validate aliases before allowing publication
The changes in the reviewed file are part of a complete implementation that prevents alias conflicts between scenes and stories through proper validation mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alias-related validations or constraints
rg -A 5 'alias.*validation|alias.*unique|alias.*conflict'
# Search for story/scene alias handling
ast-grep --pattern 'function $_(story, scene) {
$$$
alias
$$$
}'
Length of output: 132
Script:
#!/bin/bash
# Search for alias-related code in story and scene files
rg -g '**/*.{ts,tsx}' -B 2 -A 5 'alias.*story|story.*alias|alias.*scene|scene.*alias'
# Search for story/scene publish-related mutations and validations
rg -g '**/*.{ts,tsx,graphql}' -B 2 -A 5 'mutation.*Story|mutation.*Scene|validate.*Story|validate.*Scene'
# Look for GraphQL schema definitions related to aliases
fd -e graphql -x cat {} | grep -A 5 -B 2 'type.*Story\|type.*Scene\|input.*Story\|input.*Scene'
Length of output: 227018
Script:
#!/bin/bash
# Search for alias validation or uniqueness checks in the backend
rg -g '**/*.{ts,tsx,graphql}' -B 2 -A 5 'checkProjectAlias|checkStoryAlias|validateAlias|uniqueAlias'
# Search for alias-related error handling
rg -g '**/*.{ts,tsx}' -B 2 -A 5 'alias.*error|alias.*conflict|alias.*taken'
# Look for alias-related GraphQL schema validations
fd -e graphql -x cat {} | grep -A 5 -B 2 'directive.*unique|validate.*alias|constraint.*alias'
Length of output: 36715
web/src/beta/features/ProjectSettings/hooks.ts (1)
Line range hint 1-117: Verify alignment with PR objectives.
The PR objectives mention fixing an issue where "scenes and stories could use the same alias by mistake", but the changes in this file focus on unpublishing workflow. Could you clarify how these changes address the alias conflict issue?
#!/bin/bash
# Search for alias-related code to understand the connection
rg "alias" --type tsweb/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (3)
5-5: LGTM: Centralized status transformation.
The import of toPublishmentStatus from a shared location improves consistency in status handling across the application.
Line range hint 1-324: Request: Additional context needed for alias conflict fix.
While the changes in this file improve status handling and project management, I don't see direct changes addressing the PR's main objective of preventing scenes and stories from sharing the same alias. Could you please:
- Provide the files containing the actual alias validation changes
- Explain how these changes support the alias conflict fix
Line range hint 250-270: Verify unpublishing logic during project removal.
The project removal logic correctly unpublishes "limited" status projects before deletion, which helps maintain data consistency.
Let's verify the alias conflict fix:
✅ Verification successful
Project unpublishing logic during removal is correctly implemented
The code review comment is accurate. The verification confirms that:
- The project removal logic in
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.tscorrectly checks for "limited" status projects and unpublishes them before deletion:
if (project?.status === "limited") {
await usePublishProject("unpublished", project.id);
}
await useUpdateProjectRemove(updatedProject);-
The implementation is consistent across the codebase, with similar unpublishing logic present in other project removal flows (e.g., in
ProjectSettings/hooks.ts). -
The
PublishmentStatustypes and state transitions are properly defined and handled throughout the backend and frontend code.
The unpublishing step before deletion helps maintain data consistency by ensuring published content is properly unpublished before being removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for alias-related code to verify the fix implementation
# Expected: Find code that prevents scenes and stories from sharing aliases
# Search for alias-related validations
rg -A 5 'alias.*validation|validateAlias|checkAlias'
# Search for GraphQL mutations related to aliases
ast-grep --pattern 'mutation {
$$$
alias
$$$
}'
Length of output: 110
Script:
#!/bin/bash
# Let's verify the unpublishing logic during project removal
# Search for unpublishing and project removal related code
rg -A 10 'usePublishProject|useUpdateProjectRemove'
# Search for project status related code
rg -A 5 '"limited"|ProjectStatus'
# Search for project deletion mutations
ast-grep --pattern 'mutation {
$$$
deleteProject
$$$
}'
Length of output: 30378
web/src/services/i18n/translations/en.yml (2)
201-205: LGTM! Clear differentiation between scene and story publishing.
The new translations effectively separate and clarify the publishing workflows for scenes and stories, which aligns well with the PR objective of preventing alias conflicts. The messages are:
- Consistent in structure
- Clear in communicating the implications of each action
- Well-differentiated between scene and story contexts
202-205: Verify consistent usage of publishing messages across the application.
The new publishing-related messages introduce clear distinctions between scenes and stories. Let's verify that these messages are used consistently across the application.
✅ Verification successful
Publishing messages are consistently used across the application
The verification shows that the publishing-related messages are used consistently:
- All messages are properly defined in both English (en.yml) and Japanese (ja.yml) translation files
- The messages are correctly used in the UI components under
web/src/beta/features/Editor/Publish/:PublishOrUpdateModaluses the publish/update messagesUnpublishModaluses the unpublish messages- Each component correctly differentiates between scene and story contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of scene/story publishing messages
# Expected: Each message should be used in appropriate scene/story contexts
# Search for scene/story publishing related code
rg -l "publish.*(?:scene|story)" --type=tsx --type=ts
# Search for potential mixed usage of scene/story terms
rg -l "(?:scene|story).*alias" --type=tsx --type=ts
Length of output: 165
Script:
#!/bin/bash
# Let's try with simpler regex patterns and without file type restrictions
# Search for publishing-related code with scene/story mentions
rg -l "publish.*scene|publish.*story"
# Also search for the exact translation keys to verify their usage
rg -i "Your (published|scene|story).*(published|updated)"
Length of output: 4579
web/src/services/api/projectApi.ts (1)
41-42: LGTM! Centralization of publish-related types.
The consolidation of PublishStatus type and toGqlStatus function into publishTypes improves code organization and type safety.
Let's verify that all files using these types have been updated:
✅ Verification successful
Import changes are correctly implemented across the codebase
All files using PublishStatus and toGqlStatus have been updated to import from the new centralized location at publishTypes.ts:
web/src/services/api/projectApi.tsweb/src/services/api/storytellingApi/index.tsweb/src/beta/features/Dashboard/type.ts
The implementation in publishTypes.ts is complete with proper type definitions and conversion functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old imports of PublishStatus
rg "import.*PublishStatus.*from.*@reearth/beta/features/Editor/Publish/PublishToolsPanel/PublishModal/hooks"
# Search for any remaining old imports of toGqlStatus
rg "import.*toGqlStatus.*from.*./toGqlStatus"
Length of output: 158
Script:
#!/bin/bash
# Let's try a broader search for PublishStatus and toGqlStatus usage
echo "=== Searching for PublishStatus imports ==="
rg "import.*PublishStatus" -A 1
echo -e "\n=== Searching for toGqlStatus usage ==="
rg "toGqlStatus" -A 1
echo -e "\n=== Checking publishTypes.ts content ==="
fd -g "publishTypes.ts" --exec cat {}
Length of output: 2349
web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts (4)
11-20: Well-Defined PublishItem Type Enhances Type Safety
The PublishItem type is correctly defined with all necessary properties, improving type safety and maintainability of the code.
41-65: Efficient Use of useMemo for publishItems Construction
The useMemo hook is appropriately used to memoize the publishItems array, enhancing performance by avoiding unnecessary recalculations. The mapping logic for projects and stories is sound and aligns with the intended functionality.
73-84: Correct Implementation of handlePublishItemSelect
The handlePublishItemSelect function accurately updates the active sub-project state. The use of useCallback optimizes performance by memoizing the function.
87-88: State Management for Modals is Properly Handled
The initialization of unpublishModalVisible and publishModalVisible using useState is appropriate, ensuring correct state management for modal visibility.
web/src/beta/features/Editor/hooks/index.ts (2)
99-111: Integration of useStorytelling hook looks good
The introduction of the useStorytelling hook and the destructured variables enhances the storytelling functionality within the editor. The implementation appears correct and integrates well with the existing code.
136-136: Ensure useUI handles undefined storyId
Since selectedStory might be undefined, selectedStory?.id could result in storyId being undefined. Ensure that useUI correctly handles an undefined storyId to prevent potential runtime errors.
web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx (3)
87-108: Handle potential state synchronization issues with modals
Ensure that the state variables publishModalVisible and unpublishModalVisible are properly synchronized with the modal components to prevent unexpected behavior, such as modals not opening or closing correctly.
113-124: Verify props for new modal components
The UnpublishModal and PublishOrUpdateModal components are rendered with props publishItem and onClose. Ensure these components expect these props and handle them appropriately.
72-84: Ensure unique keys in list rendering
When rendering lists in React, it's important to provide unique keys to each item. Verify that item.id is unique across all publishItems to prevent potential rendering issues.
Run the following script to check for duplicate item.id values:
✅ Verification successful
No issues with key uniqueness in list rendering
Looking at the implementation in hooks.ts, the publishItems array is constructed with guaranteed unique IDs:
- For scene type: uses the project's ID
- For story type: uses each story's ID
- The array is a combination of exactly one scene item and story items from different stories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all item IDs in publishItems are unique.
# Expected: No duplicate IDs should be found.
rg -U 'publishItems\s*=\s*\[([^\]]+)\]' -o | \
grep -oP 'id:\s*"\K[^"]+' | \
sort | uniq -d
Length of output: 89
Script:
#!/bin/bash
# Let's try to find the definition of publishItems to understand its structure
ast-grep --pattern 'const publishItems = $_'
# Also search for any array assignments to publishItems
rg 'publishItems\s*=' -A 5
Length of output: 75
Script:
#!/bin/bash
# Let's try to find the file first and then examine its content
fd "PublishToolsPanel" --type f --exec cat {}
# Also search for any files that might contain publishItems definition
fd "index.tsx" --type f --exec rg -l "publishItems" {}
Length of output: 169
Script:
#!/bin/bash
# Let's examine the content of the specific file we found
cat ./web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx
Length of output: 5205
Script:
#!/bin/bash
# Let's examine the hooks file to see how publishItems is defined
cat ./web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts
# Also check for any related types or interfaces
rg -t typescript "interface.*PublishItem" --type-add "typescript:*.{ts,tsx}"
Length of output: 3148
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts
Outdated
Show resolved
Hide resolved
...ta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (16)
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx (2)
21-23: Use semantic heading level for subtitleConsider extending the Typography component with a proper heading level for better document structure:
-export const Subtitle = styled(Typography)({ +export const Subtitle = styled(Typography).attrs({ + variant: "h3", + component: "h3" +})({ textAlign: "left" });
57-64: Use theme-based dimensions and improve responsivenessReplace magic numbers with theme-based values and consider responsive design:
export const UrlAction = styled("div")(() => ({ display: "flex", alignItems: "center", justifyContent: "center", - width: 35, - minHeight: 18, + width: theme.spacing.extraLarge, + minHeight: theme.spacing.large, flexShrink: 0 }));web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx (2)
20-21: Follow React hook naming conventions.The destructured hook aliases drop the "use" prefix, which violates React hook naming conventions. Consider keeping the "use" prefix for clarity:
- const { usePublishProject: publishProject } = useProjectFetcher(); - const { usePublishStory: publishStory } = useStorytellingFetcher(); + const { usePublishProject } = useProjectFetcher(); + const { usePublishStory } = useStorytellingFetcher();
35-48: Add loading indicator to Unpublish button.The Unpublish button should show a loading indicator while the operation is in progress.
<Button title={t("Unpublish")} appearance="primary" onClick={handleUnpublish} disabled={isLoading} + loading={isLoading} />web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx (3)
10-15: Consider type refinements for better type safety.The Props type could be more specific:
- Consider making
publicUrloptional since the render logic handles the empty case- Consider being explicit about required fields in
PublishItemtypetype Props = { publishItem: PublishItem; - publicUrl: string; + publicUrl?: string; searchEngineIndexEnabled: boolean; handleSearchIndexEnableChange: (checked: boolean) => void; };
29-45: Simplify the description text logic for better readability.The nested ternaries make the code harder to maintain. Consider extracting the logic into a more readable format.
const updateDescriptionText = useMemo(() => { - return publishItem.isPublished - ? publishItem.type === "story" - ? t( - `Your published story will be updated. This means all current changes will overwrite the current published story.` - ) - : t( - `Your published scene will be updated. This means all current changes will overwrite the current published scene.` - ) - : publishItem.type === "story" - ? t( - `Your story will be published. This means anybody with the below URL will be able to view this story.` - ) - : t( - `Your scene will be published. This means anybody with the below URL will be able to view this scene.` - ); + const itemType = publishItem.type === "story" ? "story" : "scene"; + const action = publishItem.isPublished ? "updated" : "published"; + const message = publishItem.isPublished + ? t(`Your published ${itemType} will be updated. This means all current changes will overwrite the current published ${itemType}.`) + : t(`Your ${itemType} will be published. This means anybody with the below URL will be able to view this ${itemType}.`); + return message; }, [t, publishItem]);
58-68: Improve type safety in settings navigation.The optional chaining on
handleNavigationToSettingsis unnecessary since the hook always returns a function. Also, consider making the story ID handling more explicit.<Button size="small" onClick={() => - handleNavigationToSettings?.( + handleNavigationToSettings( "public", - publishItem.type === "story" ? publishItem.storyId : "" + publishItem.type === "story" && publishItem.storyId ? publishItem.storyId : undefined ) } title={t("Go to settings")} />web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts (1)
41-65: Consider centralizing alias validation logicTo ensure consistent alias validation across the application, consider:
- Creating a centralized alias validation service
- Implementing a hook (e.g.,
useAliasValidation) that handles all alias-related checks- Adding a cache or memoization for alias lookups to improve performance
This would make it easier to maintain and extend alias validation rules in the future.
web/src/beta/features/Editor/hooks/useUI.ts (3)
42-53: Consider adding a comment explaining the default scene type choiceWhile the initialization logic is correct, it would be helpful to document why scenes are chosen as the default type when only
projectIdis available.// Subproject = scene project or story project + // Default to scene type when only projectId is available as scenes are the base content type const [activeSubProject, setActiveSubProject] = useState< SubProject | undefined >(
61-90: Add default case and improve documentationThe effect logic is sound but could be more robust:
- Add a default case to the switch statement for future maintainability
- Document the early return condition
- if (!projectId) return; + // Skip effect if no project context is available + if (!projectId) return; + switch (tab) { case "story": if (storyId) { setActiveSubProject((prev) => prev?.id === storyId ? prev : { id: storyId, type: "story", projectId, storyId } ); } break; case "map": case "widgets": setActiveSubProject((prev) => prev?.id === projectId ? prev : { id: projectId, type: "scene", projectId } ); break; + default: + console.warn(`Unhandled tab type: ${tab}`); }
179-180: Consider grouping related return valuesFor better readability, consider grouping related values in the return object. The
activeSubProjectand its handler could be placed together with other project-related values.web/src/beta/features/Editor/hooks/index.ts (2)
104-104: Remove commented-out codeThe comment about unused
handlePageDuplicateshould be removed as it doesn't provide value and may become outdated.- // handlePageDuplicate // not in use
Line range hint
332-359: Consider organizing return properties by featureThe return object contains a mix of properties from different features. Consider grouping related properties together for better maintainability. For example:
return { // Visualizer props visualizerSize, isVisualizerResizing, visualizerRef, handleIsVisualizerUpdate, handleCoreAPIReady, // Story props storyPanelRef, activeSubProject, selectedStory, installableStoryBlocks, handleStoryBlockMove, selectStoryPage, // Layer props selectedLayer, layerStyles, layers: nlsLayers, layerId, handleLayerAdd, handleCustomPropertySchemaUpdate, // Widget props showWASEditor, selectedWidgetArea, selectWidgetArea, // Context values mapPageValue, storyPageValue, widgetsPageValue, publishPageValue, // UI state dataSourceLayerCreatorShown, sketchLayerCreatorShown, customPropertySchemaShown, // Handlers handleSketchTypeChange, handleSketchFeatureCreate, handleSketchFeatureUpdate, handleCoreLayerSelectFromMap, closeDataSourceLayerCreator, closeSketchLayerCreator, closeCustomPropertySchema, }web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx (3)
85-112: Optimize dependency arrays inuseMemoanduseCallbackhooks.Some variables in the dependency arrays are functions or objects that may cause unnecessary re-renders because they change on every render due to reference inequality.
For instance, in the
actionsuseMemo(lines 85-112):
onCloseandhandleProjectPublishare functions and could be memoized usinguseCallbackto prevent unnecessary re-computations.primaryButtonTextandsecondaryButtonTextare strings computed viauseMemo; ensure their dependencies are as minimal as necessary.Consider the following adjustments:
// Memoize the onClose callback if possible -const PublishOrUpdateModal: FC<Props> = ({ publishItem, onClose }) => { +const PublishOrUpdateModal: FC<Props> = ({ publishItem, onClose }) => { + const memoizedOnClose = useCallback(onClose, []); // Memoize the handleProjectPublish function -const handleProjectPublish = useCallback(async () => { +const handleProjectPublish = useCallback(async () => { // existing code -}, [ +}, [alias, searchEngineIndexEnabled, publishItem, publishProject, publishStory]); // Update the actions useMemo dependencies -[ - onClose, +[ + memoizedOnClose, handleProjectPublish, aliasValid, primaryButtonText, secondaryButtonText, publishDone, isPublishing, ]
64-75: Simplify nested ternary operators for better readability.The
titlecomputation contains multiple nested ternary operators, which can be difficult to read and maintain.Refactor the logic using
if-elsestatements for clarity:const title = useMemo(() => { if (publishDone) { return t("Congratulations!"); } if (!publishItem.isPublished) { return publishItem.type === "story" ? t("Publish your story") : t("Publish your scene"); } return publishItem.type === "story" ? t("Update your story") : t("Update your scene"); }, [t, publishDone, publishItem]);
76-83: Avoid unnecessaryuseMemofor simple value computations.The
primaryButtonTextandsecondaryButtonTextvariables are simple string values derived from props and state. UsinguseMemohere may not provide a significant performance benefit and can add complexity.Consider defining them without
useMemo:const primaryButtonText = !publishItem.isPublished ? t("Publish") : t("Update"); const secondaryButtonText = !publishDone ? t("Cancel") : t("Ok");Also applies to: 80-84
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishedOrUpdatedSection.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts(1 hunks)web/src/beta/features/Editor/Publish/context.tsx(1 hunks)web/src/beta/features/Editor/hooks/index.ts(4 hunks)web/src/beta/features/Editor/hooks/useUI.ts(3 hunks)web/src/services/i18n/translations/ja.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishedOrUpdatedSection.tsx
- web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts
- web/src/beta/features/Editor/Publish/context.tsx
🔇 Additional comments (24)
web/src/beta/features/Editor/Publish/PublishToolsPanel/common.tsx (2)
30-41: Move gap property to a more appropriate component
The gap property might cause unexpected spacing when the wrapper contains elements that need different spacing rules. Consider moving it to a child container if needed.
Note: Previous review comments about adding hover and focus states are still applicable.
43-55: 🛠️ Refactor suggestion
Simplify cursor behavior logic
The cursor property is set twice - once in the parent UrlWrapper and once conditionally here. This could lead to confusing behavior:
export const UrlText = styled("div")<{ hasPublicUrl?: boolean }>(
({ hasPublicUrl, theme }) => ({
display: "flex",
justifyContent: "center",
alignItems: "center",
fontSize: theme.fonts.sizes.body,
whiteSpace: "break-spaces",
color: hasPublicUrl ? theme.primary.main : "inherit",
fontWeight: hasPublicUrl ? "bold" : "normal",
- cursor: hasPublicUrl ? "pointer" : "default"
})
);Note: Previous review comments about text truncation and using semantic HTML are still applicable.
web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx (2)
1-15: LGTM! Clean imports and type definitions.
The imports are well-organized and the Props type is properly defined with clear type safety.
26-30: Verify alias handling during unpublish operations.
The PR aims to fix alias conflicts between scenes and stories. Let's verify that the unpublish operations properly handle aliases.
✅ Verification successful
Let me gather more specific information about alias handling during unpublish operations.
Let me check the GraphQL mutations and the actual implementation of publish/unpublish operations.
Let me check the implementation of alias handling in the publish/unpublish operations.
Based on the code analysis, I can now provide a final response regarding the alias handling during unpublish operations.
Unpublish operation properly handles aliases
The unpublish operation is correctly implemented and doesn't need additional alias conflict handling because:
- When unpublishing, the GraphQL mutation
publishStory/publishProjectis called with status "unpublished", which inherently releases the alias on the backend - The alias handling is managed through the
useAliashook which:- Validates aliases for uniqueness using
checkProjectAlias - Maintains alias state separately for stories and projects
- Automatically generates new unique aliases when needed
- Validates aliases for uniqueness using
The implementation ensures that aliases are properly released when content is unpublished, preventing any potential conflicts when the same alias is reused later.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alias-related code in publish/unpublish operations
echo "Searching for alias handling in publish operations..."
rg -l 'alias.*publish' --type ts
# Search for alias conflict prevention logic
echo "Searching for alias conflict prevention..."
ast-grep --pattern 'function $_($$_) {
$$$
alias
$$$
}'
Length of output: 624
Script:
#!/bin/bash
# Check the implementation of publishStory and publishProject functions
echo "Searching for publishStory and publishProject implementations..."
rg -A 10 "export (const|function) publish(Story|Project)" --type ts
# Check for alias-related mutations in GraphQL
echo "Searching for alias-related mutations..."
rg "mutation.*\balias\b.*unpublish" --type graphql --type ts
# Check the hooks implementation for alias handling
echo "Checking hooks implementation..."
cat "web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts"
Length of output: 3557
Script:
#!/bin/bash
# Check GraphQL mutations for publish/unpublish
echo "Checking GraphQL mutations..."
rg -A 10 "mutation.*\b(publish|unpublish)" web/src/services/gql/
# Check the implementation of publish functions
echo "Checking publish function implementations..."
rg -A 10 "publish(Story|Project|Scene)" web/src/services/api/
# Check for alias validation logic
echo "Checking alias validation..."
rg -A 5 "validateAlias|checkAlias|aliasConflict" --type ts
Length of output: 150159
Script:
#!/bin/bash
# Check for alias handling in the hooks implementation
echo "Checking hooks implementation for alias handling..."
cat "web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts"
# Check for alias-related code in the publish operations
echo "Checking publish operations for alias handling..."
rg -A 10 "alias.*publish" web/src/beta/features/Editor/Publish/PublishToolsPanel/
# Check for alias validation in the API layer
echo "Checking API layer for alias validation..."
rg -A 10 "alias.*check" web/src/services/api/
Length of output: 6581
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/PublishOrUpdateSection.tsx (3)
17-22: LGTM!
The component definition and props destructuring follow React best practices.
52-54: Add security attributes to external URL opening.
This issue was previously identified and remains valid. When opening external URLs, security attributes should be added.
1-79: Verify alias conflict prevention.
While this component handles the UI for publishing items, it doesn't address the PR's main objective of preventing scenes and stories from sharing the same alias. Consider adding validation or checks to ensure alias uniqueness.
web/src/beta/features/Editor/Publish/PublishToolsPanel/hooks.ts (5)
22-34: LGTM: Clean hook parameter structure
The hook parameters and data fetching setup follow React best practices.
73-84: LGTM: Well-structured selection handling
The selection handling is properly memoized and follows React best practices.
87-97: LGTM: Clean modal state management
The modal visibility state management is simple and effective.
54-63: Verify story alias uniqueness across all stories
While line 60 prevents stories from using the project's alias, it doesn't prevent multiple stories from sharing the same alias. Consider adding a check to ensure uniqueness across all story aliases.
Let's verify if there are any existing story alias collision checks:
#!/bin/bash
# Search for story-related alias handling
ast-grep --pattern 'story.$$$alias'
rg -A 5 "story.*alias|alias.*story"11-19: Consider strengthening alias validation in PublishItem type
Given the PR objective to prevent scenes and stories from sharing the same alias, consider adding validation or constraints to the alias property type to enforce uniqueness.
Let's check how aliases are validated elsewhere in the codebase:
web/src/beta/features/Editor/hooks/useUI.ts (4)
11-16: Well-structured type definition for preventing scene/story conflicts
The SubProject type effectively differentiates between scenes and stories through strict typing, helping prevent the alias conflict issue mentioned in the PR objectives.
19-20: LGTM: Props type properly extended
The addition of optional projectId and storyId to Props aligns well with the new SubProject type structure.
55-57: LGTM: Clean state management implementation
The handler is properly memoized and follows React best practices.
Line range hint 11-90: Verify all usages of the new SubProject type
Let's ensure all components using this hook have been updated to handle the new SubProject type correctly.
✅ Verification successful
All usages of SubProject type have been properly updated
The verification shows that:
- The new
SubProjecttype is consistently used across the codebase - All references to the old
currentProjectTypeare commented out - The
SubProjecttype and its handlers are properly imported and used in all relevant components:- Editor component
- Publish context and tools
- Various hook implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using the useUI hook
rg -l "useUI" --type ts --type tsx
# Search for any remaining references to the old project type management
rg "currentProjectType|handleProjectTypeChange" --type ts --type tsx
Length of output: 161
Script:
#!/bin/bash
# Search for components using the useUI hook without file type restrictions
rg -l "useUI"
# Search for any remaining references to the old project type management
rg "currentProjectType|handleProjectTypeChange"
# Search for usages of SubProject type
rg "SubProject"
Length of output: 3960
web/src/beta/features/Editor/hooks/index.ts (2)
133-134: LGTM! Safe handling of story ID
The optional chaining on selectedStory?.id properly handles cases where no story is selected.
303-318: Clean up commented-out code and verify refactoring
- Remove all commented-out code related to the old project type implementation.
- The transition from project types to subprojects needs verification to ensure all references are updated.
web/src/services/i18n/translations/ja.yml (2)
201-201: Translation added as requested.
The missing translation for scene embedding code explanation has been added, addressing the previous review comment.
202-205: LGTM! Clear and consistent translations.
The translations for publishing and updating scenes/stories are well-structured and maintain consistency between scene and story contexts. They effectively communicate the implications of the actions to users.
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/index.tsx (4)
37-54: Add error handling to the publish functions.
The handleProjectPublish function does not currently handle errors from publishStory or publishProject. If these functions fail, it could result in unhandled promise rejections and a poor user experience. Consider adding error handling to manage potential errors gracefully and provide feedback to the user.
114-122: Ensure config().published contains '{}' placeholder when constructing publicUrl.
The publicUrl construction assumes that publishedConfig includes the '{}' placeholder. If this placeholder is missing, suffix may be undefined, leading to incorrect URLs. Consider adding validation to handle cases where the placeholder is absent or malformed.
119-119: Sanitize alias to remove all slashes when constructing publicUrl.
The current code alias?.replace(/^\/+|\/+$/g, "") ?? "" only removes leading and trailing slashes from the alias. If alias contains slashes in the middle, they will not be removed, potentially causing issues with the generated URL. Use a regular expression to remove all slashes from alias.
26-28: Verify that fetching hooks are correctly implemented.
Ensure that the destructured functions usePublishProject and usePublishStory are correctly named and used. The current destructuring might lead to confusion or incorrect references if the imported hooks do not match the naming.
Run the following script to check the exports from useProjectFetcher and useStorytellingFetcher:
✅ Verification successful
Based on the search results, I can now verify the implementation. The code shows that both useProjectFetcher and useStorytellingFetcher are correctly imported and used in the file. The search results confirm that:
- Both hooks are properly exported from
services/api/index.ts - The destructuring syntax is used correctly in multiple files including the one under review
- The hooks
usePublishProjectandusePublishStoryare consistently used across the codebase in similar contexts (e.g., inPublishOrUpdateModal/index.tsxandUnpublishModal/index.tsx)
The fetching hooks are correctly implemented and used.
The destructuring syntax in the code is consistent with the codebase's patterns and the hooks are properly imported from their respective fetchers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the exported members from useProjectFetcher and useStorytellingFetcher.
# Expected: Both modules should export functions named usePublishProject and usePublishStory.
# Check useProjectFetcher
rg 'export function usePublishProject' src/services/api/projects.ts
# Check useStorytellingFetcher
rg 'export function usePublishStory' src/services/api/stories.ts
Length of output: 273
Script:
#!/bin/bash
# Let's first find the correct paths for these fetchers
fd -e ts -e tsx "useProjectFetcher|useStorytellingFetcher"
# Then search for any exports containing publish related functions
rg -g '*.{ts,tsx}' 'export.*use.*Publish(Project|Story)'
Length of output: 119
Script:
#!/bin/bash
# Let's try to find these fetcher files with a broader search
rg -g '*.{ts,tsx}' "useProjectFetcher|useStorytellingFetcher" -A 5 -B 5
# Also search for any file that might export these hooks
rg -g '*.{ts,tsx}' "export.*\{.*usePublishProject.*\}|export.*\{.*usePublishStory.*\}"
Length of output: 26079
web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx
Show resolved
Hide resolved
web/src/beta/features/Editor/Publish/PublishToolsPanel/UnpublishModal/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
web/src/beta/features/ProjectSettings/hooks.ts (1)
Line range hint
93-116: Fix disabled state management and add error handling.The disabled state is toggled before async operations complete, and there's no error handling for the unpublish operation.
Apply this diff to fix the issues:
const handleProjectRemove = useCallback(async () => { - const updatedProject = { - projectId, - deleted: true - }; setDisabled(!disabled); + try { + await unpublish(); - await unpublish(); + const updatedProject = { + projectId, + deleted: true + }; - const { status } = await useUpdateProjectRemove(updatedProject); - client.cache.evict({ - id: client.cache.identify({ - __typename: "Project", - id: projectId - }) - }); - client.cache.gc(); - if (status === "success") { - navigate(`/dashboard/${workspaceId}/`); - } + const { status } = await useUpdateProjectRemove(updatedProject); + client.cache.evict({ + id: client.cache.identify({ + __typename: "Project", + id: projectId + }) + }); + client.cache.gc(); + if (status === "success") { + navigate(`/dashboard/${workspaceId}/`); + } + } catch (error) { + console.error('Failed to remove project:', error); + } finally { + setDisabled(false); + } }, [ client.cache, disabled, unpublish, navigate, projectId, useUpdateProjectRemove, workspaceId ]);web/src/beta/features/Editor/hooks/index.ts (2)
99-114: Remove commented-out code reference.The comment
// handlePageDuplicate // not in useshould be removed as it doesn't provide value and may cause confusion.
326-326: Consider adding TypeScript type for the return object.The return object includes new properties like
activeSubProjectand story-related fields. Consider defining an explicit interface for the return type to improve type safety and documentation.interface EditorHookResult { activeSubProject: SubProject; selectedStory: Story; // ... other properties }web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx (1)
139-139: Consider using theme colors instead of hardcoding HEX values inPublishStatus.Using hardcoded color values can lead to inconsistencies across different themes or when the theme changes. It's better to use predefined theme colors for maintainability and consistency.
Apply this diff to use a theme color:
backgroundColor: isPublished ? - "#24A148" + theme.content.positive : theme.content.weaker,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx(3 hunks)web/src/beta/features/Editor/hooks/index.ts(4 hunks)web/src/beta/features/Editor/hooks/useUI.ts(3 hunks)web/src/beta/features/ProjectSettings/hooks.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/Editor/hooks/useUI.ts
🔇 Additional comments (4)
web/src/beta/features/ProjectSettings/hooks.ts (1)
57-91: Verify the complete implementation of alias conflict prevention.
The PR objective mentions fixing alias conflicts between scenes and stories, but the changes in this file don't directly address this issue. Let's verify the complete implementation.
✅ Verification successful
Let me analyze the findings and generate additional verification scripts to check for alias conflict prevention implementation.
The initial search revealed the presence of alias-related code but we need to verify the specific implementation of conflict prevention between scenes and stories. Let's examine the validation logic and hooks in more detail.
Let me analyze the findings and generate additional verification scripts to check for alias conflict prevention implementation.
The initial search revealed the presence of alias-related code but we need to verify the specific implementation of conflict prevention between stories and projects. Let's examine the validation logic and error handling in more detail.
Based on the findings, I can now provide a final response about the alias conflict prevention implementation.
Alias conflict prevention is properly implemented
The code shows a clear implementation of alias conflict prevention between projects and stories:
- Stories cannot use the same alias as their parent project:
alias: s.alias && s.alias !== project.alias ? s.alias : undefined, // correct existing alias, needs republish- The system includes alias validation and uniqueness checks:
- Uses
checkProjectAliasquery to verify alias availability - Implements retry logic with up to 3 attempts to generate unique aliases
- Automatically corrects story aliases that conflict with project aliases by setting them to undefined
- The unpublish functionality in the reviewed code works in conjunction with this alias conflict prevention by properly handling the publishing status of both projects and their stories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for alias-related code to verify the complete implementation
# Search for alias-related functions and types
echo "Searching for alias-related code..."
rg -A 5 "alias" --type ts
# Search for validation logic
echo "Searching for validation logic..."
ast-grep --pattern 'function $_(alias$_) {
$$$
}'
# Search for uniqueness checks
echo "Searching for uniqueness checks..."
rg -A 5 "unique.*alias|alias.*unique" --type ts
Length of output: 197086
Script:
#!/bin/bash
# Search for alias validation and conflict prevention logic
echo "Searching for alias validation..."
rg -A 10 "checkProjectAlias" --type ts
echo "Searching for story alias validation..."
rg -A 10 "publishStory.*alias" --type ts
echo "Searching for project alias validation..."
rg -A 10 "publishProject.*alias" --type ts
# Search for alias-related hooks and utilities
echo "Searching for alias-related hooks..."
rg -A 10 "useAlias" --type ts
# Search for alias-related tests
echo "Searching for alias-related tests..."
rg -A 10 "alias.*test|test.*alias" --type ts
Length of output: 150286
Script:
#!/bin/bash
# Search for alias conflict prevention in hooks and components
echo "Searching for alias conflict prevention..."
rg -A 10 "s\.alias.*project\.alias|project\.alias.*s\.alias" --type ts
# Search for alias validation and uniqueness checks in hooks
echo "Searching for alias validation in hooks..."
rg -A 15 "useAlias" --type ts --glob "hooks.ts"
# Search for alias-related error handling
echo "Searching for alias-related error handling..."
rg -A 10 "alias.*error|error.*alias" --type ts
# Search for story alias handling
echo "Searching for story alias handling..."
rg -A 10 "story.*alias.*project|project.*alias.*story" --type ts
Length of output: 3100
web/src/beta/features/Editor/hooks/index.ts (2)
Line range hint 115-134: LGTM! Well-structured UI state management.
The addition of activeSubProject and its handler, along with the integration of storyId from selectedStory, provides a clear separation between scene and story management.
302-312: Verify the usage of activeSubProject in consumers.
The PublishPage context has been updated to include activeSubProject. Let's verify its usage in consumer components.
✅ Verification successful
Let me gather more information about the usage of activeSubProject in the codebase, particularly focusing on components that might be consuming it.
Let me search for more specific usage patterns of activeSubProject to understand how it's being consumed.
Based on the gathered information, I can now provide a final response about the verification of activeSubProject usage:
The activeSubProject context value is properly consumed and implemented
The context value is used appropriately in several components with clear patterns:
- Conditional rendering in Editor component based on
activeSubProject?.type === "story" - Proper type definition as a discriminated union with
sceneandstorytypes - Consistent handler implementation in PublishToolsPanel for managing sub-project state
- Proper memoization of context value and its handler in dependencies arrays
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components consuming PublishPageContext and verify proper usage of activeSubProject
# Search for PublishPageContext usage
rg -l "PublishPageContext" | while read -r file; do
echo "=== $file ==="
rg -A 5 "useContext.*PublishPageContext" "$file"
rg -A 5 "activeSubProject" "$file"
done
Length of output: 1583
Script:
#!/bin/bash
# Search for files that import from the context or hooks
rg -l "from.*Publish/context|from.*Editor/hooks" | while read -r file; do
echo "=== $file ==="
# Show the import statement and surrounding context
rg -A 5 "from.*Publish/context|from.*Editor/hooks" "$file"
# Show usage of activeSubProject in these files
rg -B 2 -A 5 "activeSubProject" "$file"
done
# Also search for any direct usage of activeSubProject in components
rg -B 2 -A 5 "activeSubProject.*=.*usePublishPage"
Length of output: 4301
Script:
#!/bin/bash
# Search for files containing SubProject type definition and its usage
rg -B 2 -A 5 "type.*SubProject"
# Search for components that might be conditionally rendered based on activeSubProject
rg -B 2 -A 5 "activeSubProject\?.type"
# Search for any handlers or effects that use activeSubProject
rg -B 2 -A 5 "handleActiveSubProjectChange|useEffect.*activeSubProject"
Length of output: 8736
web/src/beta/features/Editor/Publish/PublishToolsPanel/index.tsx (1)
70-74: Verify that the 'Update' action triggers the correct modal behavior.
Both the 'Publish' and 'Update' buttons open the PublishOrUpdateModal by setting setPublishModalVisible(true). Ensure that the modal correctly differentiates between publishing new content and updating existing content to provide the appropriate user experience.
Overview
Publish story and scene could use the same alias which is not expected. It's coming from the unproper management of the state.
What I've done
NOTE:
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
PublishOrUpdateModalfor handling publishing and updating items.UnpublishModalfor managing unpublishing actions.PublishOrUpdateSectionandPublishedOrUpdatedSectionto enhance user feedback on publishing status.Improvements
Bug Fixes