-
Notifications
You must be signed in to change notification settings - Fork 11
refactor: deployment variables #580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c9f4c08
cf616b5
c87c628
ad12714
e17939b
0e85e68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -249,7 +249,7 @@ export default async function VariablesPage(props: { | |||||||||
| deploymentVariables.map((v) => ( | ||||||||||
| <TableRow key={v.key}> | ||||||||||
| <TableCell>{v.key}</TableCell> | ||||||||||
| <TableCell>{String(v.value.value)}</TableCell> | ||||||||||
| <TableCell></TableCell> | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve user experience with empty value cell. The deployment variable value cell is now completely empty, which could confuse users. Consider:
This appears to be an incomplete implementation that needs user-facing improvements. - <TableCell></TableCell>
+ <TableCell className="text-muted-foreground">
+ <span className="italic">Value display pending refactor</span>
+ </TableCell>📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will address in separate PR
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adityachoudhari26 That sounds like a good approach to keep this PR focused on the refactoring! Would you like me to open a GitHub issue to track implementing proper deployment variable value display using the new direct/reference structure? This would help ensure the UI improvement doesn't get lost. |
||||||||||
| <TableCell> | ||||||||||
| <Link | ||||||||||
| href={urls | ||||||||||
|
|
||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Non-functional component still being used in the UI
The
EditVariableValueDialogcomponent has been replaced with a stub that returnsnull, but it's still being used in the dropdown menu (lines 93-105). This creates a confusing user experience where the "Edit" button is visible but does nothing when clicked.Either remove the usage of this component from the dropdown menu or add a TODO comment explaining when the implementation will be restored:
Or remove the edit option entirely:
<DropdownMenuContent> <DropdownMenuGroup> - <EditVariableValueDialog - value={value} - variable={variable} - onClose={() => setOpen(false)} - > - <DropdownMenuItem - onSelect={(e) => e.preventDefault()} - className="flex items-center gap-2" - > - <IconPencil className="h-4 w-4" /> - Edit - </DropdownMenuItem> - </EditVariableValueDialog> <DeleteVariableValueDialog🏁 Script executed:
Length of output: 668
Address non-functional EditVariableValueDialog usage
The
EditVariableValueDialogis a stub returningnullbut still rendered in the dropdown (lines 93–105), so the “Edit” option does nothing and will confuse users.Please either implement the dialog (or add a clear TODO) or remove its usage entirely:
• File:
apps/webservice/src/app/[workspaceSlug]/…/VariableValueDropdown.tsx– Definition at lines 28–35
– Usage at lines 93–105
Option 1: Add a TODO in the stub
const EditVariableValueDialog: React.FC<{ value: VariableValue; variable: schema.DeploymentVariable; onClose: () => void; children: React.ReactNode; }> = () => { + // TODO: implement EditVariableValueDialog after refactoring variable values return null; };Option 2: Remove the edit menu item
<DropdownMenuContent> <DropdownMenuGroup> - <EditVariableValueDialog - value={value} - variable={variable} - onClose={() => setOpen(false)} - > - <DropdownMenuItem - onSelect={(e) => e.preventDefault()} - className="flex items-center gap-2" - > - <IconPencil className="h-4 w-4" /> - Edit - </DropdownMenuItem> - </EditVariableValueDialog> <DeleteVariableValueDialog value={value} variable={variable}📝 Committable suggestion
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will address in separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityachoudhari26 Understood! That makes sense to keep this PR focused on the variable refactoring.
Would you like me to create a GitHub issue to track the EditVariableValueDialog fix as a follow-up task? I can include the details about the non-functional component and reference this PR and comment.