feat(ui): project variable support [FLOW-FE-138]#692
Conversation
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify project 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: 5
🧹 Nitpick comments (8)
ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/ProjectVarDialog.tsx (1)
159-161: Provide user feedback when removing whitespace from variable namesCurrently, whitespace in variable names is automatically removed without user feedback, which might confuse users. Instead of silently modifying the input, consider preventing whitespace entry or displaying a validation message.
You could update the input to prevent whitespace characters:
<Input value={variable.name} onClick={(e) => e.stopPropagation()} onChange={(e) => { setProjectVariables((pvs) => { const newPvs = [...pvs]; - const newValue = e.target.value; - newPvs[idx].name = newValue.split(/\s+/).join(""); + const newValue = e.target.value.replace(/\s/g, ""); + newPvs[idx].name = newValue; return newPvs; }); }} />And optionally, inform the user that whitespace is not allowed in variable names.
ui/src/types/projectVar.ts (1)
12-13: Specify a stricter type fordefinitionand address the TODOCurrently,
definitionis typed asany, which can lead to type safety issues. Consider definingdefinitionbased onVarTypeto enhance type safety. Additionally, there's a TODO to useParameterType; implementing this would improve consistency.Would you like assistance in defining
ParameterTypeand updating theProjectVartype accordingly?ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/ProjectVariable.tsx (1)
10-15: Add accessibility improvementsThe component lacks proper accessibility attributes for better screen reader support.
- <div className={`flex items-center rounded p-1 ${className}`}> + <div + className={`flex items-center rounded p-1 ${className}`} + role="listitem" + aria-label={`Project variable ${variable.name}`}> <p className="flex-1 truncate text-sm">{variable.name}</p> <p className="flex-1 truncate text-sm dark:font-extralight"> {JSON.stringify(variable.definition)} </p> </div>ui/src/mock_data/projectVars.ts (1)
5-8: Improve mock data quality and readabilityThe mock data contains unnecessarily long values and random-looking IDs which reduce readability and maintainability.
- id: "asdfasfsdasdffsdf", - name: "project_name", - definition: "My Projectjlasjdflakjsdflkjsadflkjasdflkjasdlkjafsdlk", + id: "project_name_001", + name: "project_name", + definition: "Sample Project",ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/index.tsx (2)
15-17: Plan GraphQL implementationThe TODO comment indicates missing GraphQL integration. This should be tracked and implemented to replace the mock data.
Would you like me to help create a GitHub issue to track the GraphQL implementation task? I can provide a template with the necessary requirements and implementation steps.
27-64: Add loading state handlingThe component should handle loading states when fetching project variables (future GraphQL implementation).
const ProjectVariables: React.FC = () => { const t = useT(); const [isOpen, setIsOpen] = useState(false); + const [isLoading, setIsLoading] = useState(false); // TODO: get project variables from gql const [currentProjectVars, setCurrentProjectVars] = useState<ProjectVar[]>(MOCK_DATA); // ... existing code ... return ( <> <div className="flex h-full flex-col gap-4 p-1"> <div className="flex-1"> <div className="flex items-center pb-2"> <p className="flex-1 text-sm font-thin">{t("Key")}</p> <p className="flex-1 text-sm font-thin">{t("Value")}</p> </div> <ScrollArea> + {isLoading ? ( + <div className="flex justify-center p-4"> + <LoadingSpinner /> + </div> + ) : ( <div className="flex flex-col gap-1 overflow-y-auto"> {currentProjectVars.map((variable, idx) => ( <ProjectVariable key={variable.id} className={`${idx % 2 !== 0 ? "bg-card" : "bg-primary"}`} variable={variable} /> ))} </div> + )} </ScrollArea> </div>ui/src/features/Editor/components/LeftPanel/index.tsx (1)
99-115: Consider the tab ordering logic.While the implementation is correct, consider if the current tab order (project-vars → resources → actions-list) provides the most intuitive user experience. Common patterns typically group similar functionality together.
A potential alternative order could be:
- navigator (current first)
- actions-list (related to canvas actions)
- resources (project resources)
- project-vars (project configuration)
ui/src/lib/i18n/locales/es.json (1)
21-24: Consider using more technical terminology for "Key" in Spanish.While the translations are generally good, consider using "Llave" instead of "Clave" for "Key" as it's more commonly used in programming contexts in Spanish.
Current translations:
- "Key" → "Clave" (consider: "Llave")
- "Value" → "Valor"
- "Project Variables" → "Variables del proyecto"
- "Edit Project Variables" → "Editar variables del proyecto"
- "Resources" → "Recursos"
Also applies to: 27-28
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/ProjectVarDialog.tsx(1 hunks)ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/ProjectVariable.tsx(1 hunks)ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/index.tsx(1 hunks)ui/src/features/Editor/components/LeftPanel/components/index.ts(1 hunks)ui/src/features/Editor/components/LeftPanel/index.tsx(4 hunks)ui/src/lib/i18n/locales/en.json(1 hunks)ui/src/lib/i18n/locales/es.json(1 hunks)ui/src/lib/i18n/locales/fr.json(1 hunks)ui/src/lib/i18n/locales/ja.json(1 hunks)ui/src/lib/i18n/locales/zh.json(1 hunks)ui/src/mock_data/projectVars.ts(1 hunks)ui/src/types/index.ts(1 hunks)ui/src/types/projectVar.ts(1 hunks)
🔇 Additional comments (9)
ui/src/features/Editor/components/LeftPanel/components/index.ts (1)
3-3: LGTM!
The export statement correctly includes the ProjectVariables component.
ui/src/types/index.ts (1)
10-10: LGTM!
The export statement correctly adds projectVar types to the exports.
ui/src/mock_data/projectVars.ts (1)
32-37: Consider implementing object type support
The commented-out object type configuration suggests incomplete feature implementation. If object type support is planned, it should be properly implemented or removed to avoid confusion.
Let's check if object type is supported in the codebase:
ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/index.tsx (1)
22-25: 🛠️ Refactor suggestion
Add error handling for state updates
The handleSubmit function lacks error handling for the state update operation.
const handleSubmit = (newProjectVars: ProjectVar[]) => {
+ try {
setCurrentProjectVars(newProjectVars);
handleDialogClose();
+ } catch (error) {
+ console.error('Failed to update project variables:', error);
+ // TODO: Add error notification
+ }
};Likely invalid or redundant comment.
ui/src/features/Editor/components/LeftPanel/index.tsx (2)
2-2: LGTM: Clean import additions and type definition.
The new imports and type extension for project variables support are well-organized and follow the existing pattern.
Also applies to: 19-19, 21-21
160-160: LGTM: Improved layout structure.
The flex layout modifications and overflow handling enhance the panel's responsiveness and scrolling behavior.
Also applies to: 167-167
ui/src/lib/i18n/locales/zh.json (1)
21-28: LGTM: Complete and accurate Chinese translations.
All new keys have appropriate Chinese translations that are semantically correct and maintain consistency with the existing translations.
ui/src/lib/i18n/locales/ja.json (1)
21-24: LGTM! Japanese translations are accurate and natural.
The translations for project variables feature are well-implemented with appropriate Japanese terminology:
- "Key" → "キー"
- "Value" → "値"
- "Project Variables" → "プロジェクト変数"
- "Edit Project Variables" → "プロジェクト変数を編集"
- "Resources" → "リソース"
Also applies to: 27-28
ui/src/lib/i18n/locales/fr.json (1)
21-24: LGTM! French translations are accurate and consistent.
The translations for project variables feature are well-implemented with appropriate French terminology:
- "Key" → "Clé"
- "Value" → "Valeur"
- "Project Variables" → "Variables du projet"
- "Edit Project Variables" → "Modifier les variables du projet"
- "Resources" → "Ressources"
Also applies to: 27-28
ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/ProjectVarDialog.tsx
Outdated
Show resolved
Hide resolved
ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/ProjectVarDialog.tsx
Outdated
Show resolved
Hide resolved
ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/ProjectVariable.tsx
Outdated
Show resolved
Hide resolved
ui/src/features/Editor/components/LeftPanel/components/ProjectVariables/index.tsx
Outdated
Show resolved
Hide resolved
|
🚀 Cloud Run Preview Deployed |
1 participant
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo