feat(new-nav): add Terraform variables settings page#2546
feat(new-nav): add Terraform variables settings page#2546rmnbrd merged 10 commits intonew-navigationfrom
Conversation
|
Qovery can create a Preview Environment for this PR.
This comment has been generated from Qovery AI 🤖.
|
7fba65e to
90abb19
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## new-navigation #2546 +/- ##
=================================================
Coverage ? 44.94%
=================================================
Files ? 1057
Lines ? 21289
Branches ? 6269
=================================================
Hits ? 9569
Misses ? 9983
Partials ? 1737
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new “Terraform variables” settings page to the console-v5 settings routes, and refactors/shared-exports Terraform variable context + utilities so the service-settings UI can consume them.
Changes:
- Adds a console-v5 route for Terraform variables with provider + save action wiring.
- Re-exports Terraform variables context/utils from
@qovery/domains/service-terraform/featureand updates service-settings tests/usages accordingly. - Updates Terraform variables table + .tfvars popover styling to newer design token classnames.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/domains/service-terraform/feature/src/lib/terraform-variables-utils.ts | Updates badge class tokens for variable source display. |
| libs/domains/service-terraform/feature/src/lib/terraform-variables-context.tsx | Switches params lookup to TanStack router and renames param usage to serviceId. |
| libs/domains/service-terraform/feature/src/index.ts | Changes public exports to expose context/utils (and removes settings exports). |
| libs/domains/service-settings/feature/src/lib/terraform-variables-settings/terraform-variables-utils.spec.ts | Updates test imports to consume utils/types from service-terraform feature package. |
| libs/domains/service-settings/feature/src/lib/terraform-variables-settings/terraform-variables-table/terraform-variables-table.tsx | Restyles table and switches to shared context/utils imports from service-terraform feature. |
| libs/domains/service-settings/feature/src/lib/terraform-variables-settings/terraform-variables-table/terraform-variables-table.spec.tsx | Updates test imports for moved context/types. |
| libs/domains/service-settings/feature/src/lib/terraform-variables-settings/terraform-variables-settings.tsx | Adds a settings wrapper component rendering the variables table. |
| libs/domains/service-settings/feature/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx | Updates popover styling and context import source. |
| libs/domains/service-settings/feature/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.spec.tsx | Updates test imports for moved context/types. |
| libs/domains/service-settings/feature/src/index.ts | Exposes the variables table via service-settings feature public API. |
| apps/console-v5/src/routes/_authenticated/organization/$organizationId/project/$projectId/environment/$environmentId/service/$serviceId/settings/terraform-variables.tsx | Implements the new console-v5 Terraform variables settings page + save behavior. |
Comments suppressed due to low confidence (4)
libs/domains/service-terraform/feature/src/lib/terraform-variables-context.tsx:93
TerraformVariablesProvidernow reads params via@tanstack/react-router'suseParamsand expects aserviceIdparam. This provider is also rendered underreact-router-domroutes that use anapplicationIdparam (e.g.libs/pages/application/...page-settings-terraform-variables.tsx) and in the service creation flow, so it will either throw (no TanStack router context) or fail to resolve the correct ID. Consider making this provider router-agnostic by acceptingorganizationId/serviceId(orapplicationId) as props from the app layer, instead of importing a specific router hook inside the domain library.
libs/domains/service-settings/feature/src/lib/terraform-variables-settings/terraform-variables-table/terraform-variables-table.tsx:11TerraformVariablesTableis imported and used from the TanStack Router app (apps/console-v5/.../settings/terraform-variables.tsx), but this component depends onreact-router-dom'suseParamsto getenvironmentId. Rendering it outside a React Router context will throw. To make it usable across both routers, passenvironmentIdin as a prop (or via a context) and remove the directreact-router-domdependency from this library component.
libs/domains/service-settings/feature/src/lib/terraform-variables-settings/terraform-variables-table/terraform-variables-table.tsx:188- The error border class uses
border-surface-negative, but elsewhere the codebase uses tokenized border classes likeborder-negative-component/border-surface-negative-solid. Ifborder-surface-negativeisn't a defined Tailwind token, the error state won't render. Consider aligning with the existing negative border tokens used in other forms.
libs/domains/service-terraform/feature/src/lib/terraform-variables-utils.ts:61 getSourceBadgeClassNamewas partially migrated to design tokens, but the default/fallback branch still returns hard-coded hex colors. This leaves inconsistent styling and makes future theme changes harder; consider converting the fallback branch to the corresponding token classes as well.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ct/$projectId/environment/$environmentId/service/$serviceId/settings/terraform-variables.tsx
Show resolved
Hide resolved
libs/domains/service-terraform/feature/src/lib/terraform-variables-utils.ts
Outdated
Show resolved
Hide resolved
...e/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx
Outdated
Show resolved
Hide resolved
...e/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx
Show resolved
Hide resolved
...ct/$projectId/environment/$environmentId/service/$serviceId/settings/terraform-variables.tsx
Outdated
Show resolved
Hide resolved
...src/lib/terraform-variables-settings/terraform-variables-table/terraform-variables-table.tsx
Outdated
Show resolved
Hide resolved
...src/lib/terraform-variables-settings/terraform-variables-table/terraform-variables-table.tsx
Outdated
Show resolved
Hide resolved
...e/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx
Outdated
Show resolved
Hide resolved
...e/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx
Outdated
Show resolved
Hide resolved
...e/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx
Show resolved
Hide resolved
RemiBonnet
left a comment
There was a problem hiding this comment.
Thanks Romain 💯 !!
Quick question, when we navigate to this page we always see the loading state. Is there a reason we don’t leverage react-query’s cache here? If we do need to keep the loader, could we match the height and spacing of the final content/placeholder? Right now the spacing and the loader/icon size look a bit different!
...e/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx
Outdated
Show resolved
Hide resolved
...src/lib/terraform-variables-settings/terraform-variables-table/terraform-variables-table.tsx
Outdated
Show resolved
Hide resolved
...src/lib/terraform-variables-settings/terraform-variables-table/terraform-variables-table.tsx
Outdated
Show resolved
Hide resolved
...e/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx
Outdated
Show resolved
Hide resolved
...e/src/lib/terraform-variables-settings/terraform-tfvars-popover/terraform-tfvars-popover.tsx
Outdated
Show resolved
Hide resolved
| const TerraformVariablesWrapper = () => { | ||
| const { serviceId } = useParams({ strict: false }) | ||
| const { data: service } = useService({ serviceId }) | ||
| const methods = useForm<TerraformGeneralData>({ |
There was a problem hiding this comment.
I think it would be better to pass the TERRAFORM service as a prop. That would avoid having to use match(service) or if (service?.serviceType === 'TERRAFORM') inside this component because it's only related to terraform a not other services
5c729ea to
0061032
Compare
Summary
PR adding the "Terraform variables" settings page
Screenshots / Recordings
📺 https://www.loom.com/share/1b14e1b6f028431b889673d4b4c73faa