feat(ui): project variable support #692
Conversation
WalkthroughThis pull request introduces a comprehensive feature for managing project variables within the user interface. The changes include creating new React components for displaying and editing project variables, updating localization files across multiple languages, and defining type definitions for project variables. The implementation allows users to view, add, delete, and reorder project variables through a dialog interface, enhancing the project configuration capabilities of the application. Changes
Sequence DiagramsequenceDiagram
participant User
participant LeftPanel
participant ProjectVariables
participant ProjectVarDialog
User->>LeftPanel: Click "Project Variables" tab
LeftPanel->>ProjectVariables: Render variables
User->>ProjectVariables: Click "Edit" button
ProjectVariables->>ProjectVarDialog: Open dialog
User->>ProjectVarDialog: Add/Delete/Reorder variables
User->>ProjectVarDialog: Click "Save"
ProjectVarDialog->>ProjectVariables: Update variables
ProjectVariables->>LeftPanel: Refresh display
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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-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 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: 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
| const [projectVariables, setProjectVariables] = | ||
| useState<ProjectVar[]>(currentProjectVars); |
There was a problem hiding this comment.
Ensure projectVariables state updates when currentProjectVars prop changes
Currently, if the currentProjectVars prop changes, the projectVariables state won't update because useState doesn't synchronize with prop changes after the initial render. To keep projectVariables in sync with currentProjectVars, consider adding a useEffect hook.
Apply this diff to fix the issue:
+import { useState, useEffect } from "react";
const [projectVariables, setProjectVariables] =
useState<ProjectVar[]>(currentProjectVars);
+useEffect(() => {
+ setProjectVariables(currentProjectVars);
+}, [currentProjectVars]);Committable suggestion skipped: line range outside the PR's diff.
| const handleSubmit = () => onSubmit(projectVariables); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation before submitting project variables
Currently, there is no validation to ensure that each project variable has a non-empty name. Submitting variables with empty names might cause issues downstream. Consider adding validation to check for empty names and prevent submission if validation fails.
You can update the handleSubmit function to include validation:
const handleSubmit = () => {
+ const isValid = projectVariables.every(
+ (variable) => variable.name.trim() !== ""
+ );
+ if (!isValid) {
+ // Optionally, show an error message to the user.
+ return;
+ }
onSubmit(projectVariables);
};Additionally, you might want to disable the "Save" button when validation fails.
Committable suggestion skipped: line range outside the PR's diff.
| <p className="flex-1 truncate text-sm dark:font-extralight"> | ||
| {JSON.stringify(variable.definition)} | ||
| </p> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the display format of variable definitions
The direct use of JSON.stringify for displaying the variable definition might lead to poor readability, especially for complex types. Consider implementing a more user-friendly format display.
- <p className="flex-1 truncate text-sm dark:font-extralight">
- {JSON.stringify(variable.definition)}
- </p>
+ <p className="flex-1 truncate text-sm dark:font-extralight">
+ {formatDefinition(variable.definition, variable.type)}
+ </p>Consider adding a helper function like:
const formatDefinition = (definition: any, type: string) => {
switch (type) {
case "array":
return Array.isArray(definition) ? definition.join(", ") : String(definition);
case "boolean":
return definition ? "true" : "false";
default:
return String(definition);
}
};| {currentProjectVars.map((variable, idx) => ( | ||
| <ProjectVariable | ||
| key={variable.name + idx} | ||
| className={`${idx % 2 !== 0 ? "bg-card" : "bg-primary"}`} | ||
| variable={variable} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Fix potential key uniqueness issue
Using array index in combination with variable name as key could lead to issues if variables with the same name exist at different indices.
- <ProjectVariable
- key={variable.name + idx}
- className={`${idx % 2 !== 0 ? "bg-card" : "bg-primary"}`}
- variable={variable}
- />
+ <ProjectVariable
+ key={variable.id}
+ className={`${idx % 2 !== 0 ? "bg-card" : "bg-primary"}`}
+ variable={variable}
+ />Committable suggestion skipped: line range outside the PR's diff.
| "Key": "", | ||
| "Value": "", | ||
| "Edit Project Variables": "", | ||
| "Save": "Save", | ||
| "Transformers": "Transformers", | ||
| "Canvas Navigation": "Canvas Navigation", | ||
| "Project Variables": "", | ||
| "Resources": "", |
There was a problem hiding this comment.
Add missing translations for new keys.
Several new keys have empty string values:
- "Key"
- "Value"
- "Edit Project Variables"
- "Project Variables"
- "Resources"
These should be populated with appropriate English translations to ensure proper UI text display.
Overview
Project var (string) support with MOCK_DATA. Not hooked up to gql yet (waiting on that PR to be merged)
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo