feat: redesign Connections and Environments pages with rich tables and stepped Drawers#75
feat: redesign Connections and Environments pages with rich tables and stepped Drawers#75wicky-zipstack wants to merge 12 commits intomainfrom
Conversation
List page: - Rich table: DB icon in square tile, name + type tag + description, status (Healthy/Stale/Error), Used by (env/project counts with tooltip), Last modified, Actions (test/edit/delete inline) - Filter bar: search, database type, status dropdowns with clear + count ConnectionDrawer (new component): - Step 1: DB picker grid with real logos from API - Step 2: Name + description with validation - Step 3: Credentials with DB-specific fields via RJSF - Segmented toggle for Individual fields / Connection URL - Grid layout: host+port, user+password, database+schema side-by-side - Custom ObjectFieldTemplate with HALF_WIDTH_FIELDS set - Inline test result: success + error with collapsible View details - Friendly validation messages (Please enter Host, not raw Ajv) - Hidden ErrorList summary (inline errors only) - Drawer scoped below topbar, ESC/mask-click disabled - Reveal credentials button on URL toggle row
Backend: - Add _get_host_display() helper to extract host from decrypted connection_details - Add host, env_count, project_count to connection list API - Add host, connection_flag to environment list API connection object Environment List (rewrite): - Rich table: DB icon + name, Type tags (PROD/STG/DEV), Connection (icon + name + host), Status, Used by, Actions (test/edit/delete) - Filter bar with search, type dropdown, count - Test button uses connection test endpoint EnvironmentDrawer (new): - Info alert, General section (name + description) - Deployment Type 3-tile picker (Production/Staging/Development) - Connection dropdown with DB icon, name, type tag, host, status icon - Connection info card after selection - Deployment Credentials with Segmented toggle, RJSF grid, Reveal/Hide, inline test result with View details - Default schema field, drawer below topbar Connection List updates: - Use API host/env_count/project_count directly (removed client-side usage fetching) - Show host in connection name cell
… link - Click "Create new connection" in dropdown or "+ New connection" link next to CONNECTION heading to open ConnectionDrawer as nested drawer - On save, connection list refreshes and new connection is auto-selected - Nested drawer scoped below topbar via getContainer passthrough - Reuses same ConnectionDrawer component (zero code duplication)
Match ConnectionList pattern — refresh icon next to count on the right side of the filter bar, header only has title + New Environment button.
BE: Query UserTaskDetails and ProjectDetails counts per environment FE: Wire "Used by" column to show "X jobs · Y projects" from API data
…ponse The get_environment endpoint was missing these fields in the connection object, causing broken logo in the edit environment drawer.
|
| Filename | Overview |
|---|---|
| backend/backend/application/session/env_session.py | Adds select_related and annotated counts for env list. Introduces P1 bug: is_tested=True stamped unconditionally on update, falsely marking untested environments as Tested after metadata-only edits. |
| backend/backend/application/session/connection_session.py | Adds _get_host_display helper and annotated counts via annotate() — backend changes are clean and additive. Logger import order is unusual (placed mid-import block) but harmless. |
| frontend/src/base/components/connection/ConnectionDrawer.jsx | New stepped drawer for creating/editing connections. Logic for change detection, credential reveal, test-before-save, and RJSF schema building is well-structured. |
| frontend/src/base/components/connection/ConnectionList.jsx | Full rewrite with rich table, filter bar, inline actions, and ConnectionDrawer integration. Previously flagged dead modal and stale testingIds closure are resolved. |
| frontend/src/base/components/environment/EnvList.jsx | Full rewrite with rich env table, type tags, inline test action, and EnvironmentDrawer integration. Previously flagged stale-closure and missing deps issues are addressed. |
| frontend/src/base/components/environment/EnvironmentDrawer.jsx | New environment drawer with nested ConnectionDrawer, deployment-type tiles, RJSF credentials, and test-before-save. Two previously-flagged issues (silent plaintext encryption fallback, New connection bypass in edit mode) are still open per prior review threads. |
| frontend/src/base/components/connection/ConnectionList.css | New shared stylesheet for connection and environment pages; covers drawer positioning, dark mode, DB tile grid, and credential field layout. |
| frontend/src/base/components/connection/shared.jsx | New shared components: GridObjectFieldTemplate for RJSF grid layout and StatusTag for connection health. Clean and reusable. |
Sequence Diagram
sequenceDiagram
participant U as User
participant CL as ConnectionList
participant CD as ConnectionDrawer
participant EL as EnvList
participant ED as EnvironmentDrawer
participant API as Backend API
U->>CL: Open Connections page
CL->>API: GET /connections (annotated: env_count, project_count, host)
API-->>CL: Paginated list with counts + host
U->>CL: Click New Connection
CL->>CD: open=true
CD->>API: GET /datasources
API-->>CD: DB list with icons
CD->>API: GET /datasource_fields (on DB select)
API-->>CD: RJSF schema
U->>CD: Fill credentials → Test
CD->>API: POST /connection/test
API-->>CD: Success → canSave=true
U->>CD: Save
CD->>API: POST /connections
API-->>CD: Created
CD-->>CL: onSaved → refresh table
U->>EL: Open Environments page
EL->>API: GET /environments (annotated: job_count, project_count, host, connection_flag)
API-->>EL: Paginated list
U->>EL: Click New Environment
EL->>ED: open=true
ED->>API: GET /connections (for dropdown)
U->>ED: Select connection → credentials pre-filled
ED->>API: GET /connection/id (fetchSingleConnection)
API-->>ED: Connection details
Note over ED: Plus New connection opens nested CD
ED->>CD: open=true connectionId empty
CD-->>ED: onSaved → loadConnections + auto-select
U->>ED: Test → Save
ED->>API: POST /environments
API-->>ED: Created (is_tested=True set unconditionally)
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
backend/backend/application/session/env_session.py:155
**`is_tested` set to `True` on every save regardless of test result**
`env_model.is_tested = True` is appended unconditionally to the update path. When a user edits only the environment name or description (no credential change), the frontend's `canSave` allows saving without running a test — but the backend now stamps `is_tested = True` on the record. Any environment that previously showed "Untested" will be silently flipped to "Tested" after a name-only edit, corrupting the status tag in `EnvList`.
The fix is to only set `is_tested = True` when the credentials have actually been tested, or to preserve the existing value for metadata-only updates.
```python
# Only promote is_tested when connection_details were explicitly included in the payload
if "connection_details" in environment_details:
env_model.is_tested = True
```
Reviews (7): Last reviewed commit: "fix: set is_tested=True on environment c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| return data; | ||
| }, [connectionDataList, searchQuery, filterDb, filterStatus]); | ||
|
|
||
| /* ── Unique DB types for filter dropdown ── */ | ||
| const dbOptions = useMemo(() => { | ||
| const types = [ | ||
| ...new Set(connectionDataList.map((c) => c.datasource_name)), | ||
| ]; | ||
| return types.map((t) => ({ | ||
| label: t.charAt(0).toUpperCase() + t.slice(1), | ||
| value: t, | ||
| })); |
There was a problem hiding this comment.
filteredData is computed from connectionDataList, which holds only the items fetched for the current page. Typing "my-conn" or selecting a database type won't find anything on other pages — only the current page's rows are searched. Meanwhile the pagination footer still reports the full server-side totalCount, so it will show "50 entries across 5 pages" even when 3 items are displayed after filtering. This also means the count chip ({filteredData.length} connections) is misleading — it shows the filtered subset of one page, not matching records across all pages.
The same issue applies to EnvList.jsx with its filteredData/filterType/searchQuery state.
To fix this properly, filters should be passed as API query params (e.g. search, datasource_name, connection_flag) so the server filters the full dataset, and the pagination total can reflect the filtered count.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/base/components/connection/ConnectionList.jsx
Line: 153-165
Comment:
**Client-side filter applies only to the current page**
`filteredData` is computed from `connectionDataList`, which holds only the items fetched for the current page. Typing "my-conn" or selecting a database type won't find anything on other pages — only the current page's rows are searched. Meanwhile the pagination footer still reports the full server-side `totalCount`, so it will show "50 entries across 5 pages" even when 3 items are displayed after filtering. This also means the count chip (`{filteredData.length} connections`) is misleading — it shows the filtered subset of one page, not matching records across all pages.
The same issue applies to `EnvList.jsx` with its `filteredData`/`filterType`/`searchQuery` state.
To fix this properly, filters should be passed as API query params (e.g. `search`, `datasource_name`, `connection_flag`) so the server filters the full dataset, and the pagination total can reflect the filtered count.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Acknowledged — same pattern as JobList, EnvList, and all other paginated tables in the codebase. Server-side filtering would require API changes. Deferring to a follow-up if needed.
| loading={testingIds[record.id]} | ||
| onClick={() => handleTestConnection(record.id)} | ||
| disabled={record.is_sample_project} |
There was a problem hiding this comment.
isDeleteModalOpen is never set to truthy
The isDeleteModalOpen state and the <Modal> block at the bottom of the component are left over from the old code. The new delete flow uses <Popconfirm> directly on each row; setIsDeleteModalOpen is only ever called with false (in the modal's own onCancel). The modal therefore never opens, and onOk would call deleteConn(false) with an invalid ID if it somehow did. The state declaration and modal block can be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/base/components/connection/ConnectionList.jsx
Line: 361-363
Comment:
**Dead delete Modal — `isDeleteModalOpen` is never set to truthy**
The `isDeleteModalOpen` state and the `<Modal>` block at the bottom of the component are left over from the old code. The new delete flow uses `<Popconfirm>` directly on each row; `setIsDeleteModalOpen` is only ever called with `false` (in the modal's own `onCancel`). The modal therefore never opens, and `onOk` would call `deleteConn(false)` with an invalid ID if it somehow did. The state declaration and modal block can be removed.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in f3e25aa — removed dead Modal, isDeleteModalOpen state, and stale setIsDeleteModalOpen reference. Delete now uses inline Popconfirm only.
abhizipstack
left a comment
abhizipstack
left a comment
There was a problem hiding this comment.
Good redesign — nested drawer pattern with zero code duplication is clean, and the PR description is thorough. Three issues to address before merge (details in inline comments).
| host = details.get("host", "") | ||
| port = details.get("port", "") | ||
| return f"{host}:{port}" if host and port else host or None | ||
| if ds == "snowflake": |
There was a problem hiding this comment.
Must Fix: _get_host_display() decrypts passwords/tokens unnecessarily
con_model.decrypted_connection_details runs Fernet decryption on every sensitive field (password, api_key, token, connection_url, etc.) — but this function only reads non-sensitive fields like host, port, account, project_id.
These fields are stored as plaintext in the DB (only fields in SENSITIVE_FIELDS are encrypted). So you can read connection_details directly and skip decryption entirely:
def _get_host_display(con_model):
try:
details = con_model.connection_details # raw dict, no decryption needed
ds = con_model.datasource_name
if ds in ("postgres", "mysql", "trino"):
host = details.get("host", "")
port = details.get("port", "")
return f"{host}:{port}" if host and port else host or None
if ds == "snowflake":
return details.get("account") or None
if ds == "bigquery":
return details.get("project_id") or None
if ds == "databricks":
return details.get("host") or None
if ds == "duckdb":
return details.get("file_path") or None
except Exception:
pass
return NoneThis eliminates Fernet CPU overhead on every row in the list API (20 connections × decryption = noticeable on page load).
There was a problem hiding this comment.
Fixed in f3e25aa — now reads connection_details directly (the raw JSON) instead of decrypted_connection_details. Host, port, account, project_id are all plaintext — no Fernet decryption needed.
| connection_model_id=con_model.connection_id, is_deleted=False | ||
| ).count() | ||
| project_count = ProjectDetails.objects.filter( | ||
| connection_model_id=con_model.connection_id |
There was a problem hiding this comment.
Must Fix: N+1 queries — 3 extra DB queries per connection row
This loop fires env_count + project_count queries per row. Combined with the existing ProjectDetails.objects.filter().first() above, that's 3 queries per page item (60 queries for 20 connections).
Fix with annotate() — batch into a single query:
from django.db.models import Count, Q
con_models_qs = ConnectionDetails.objects.filter(
**filter_condition, is_deleted=False
).annotate(
env_count=Count(
'environmentmodels',
filter=Q(environmentmodels__is_deleted=False)
),
project_count=Count('projectdetails')
).order_by('-modified_at')Then access con_model.env_count directly in the loop — zero extra queries.
There was a problem hiding this comment.
Fixed in f3e25aa — replaced the per-row loop queries with a single annotate(env_count=Count(...), project_count=Count(...), is_sample=Exists(...)). Zero N+1 queries now.
| for env_model in env_models.get("page_items"): | ||
| job_count = UserTaskDetails.objects.filter( | ||
| environment=env_model | ||
| ).count() |
There was a problem hiding this comment.
Must Fix: Same N+1 pattern — 2 count queries per environment row + missing select_related
job_count and project_count fire 2 queries per row. Plus, accessing env_model.connection_model.connection_name and calling _get_host_display(env_model.connection_model) likely triggers another query per row to load the related connection (ForeignKey not prefetched).
Fix:
env_qs = EnvironmentModels.objects.filter(
**filter_condition, is_deleted=False
).select_related(
'connection_model' # eliminates ForeignKey N+1
).annotate(
job_count=Count('usertaskdetails'),
project_count=Count('projectdetails')
).order_by('-modified_at')select_related('connection_model') loads the connection in the same query. annotate() batches the counts. Total: 1 query instead of ~60.
There was a problem hiding this comment.
Fixed in f3e25aa — added select_related('connection_model') + annotate(job_count=Count(...), project_count=Count(...)). Single query for all data including connection fields.
|
Should Improve: ConnectionDrawer.jsx (915 lines) and EnvironmentDrawer.jsx (1103 lines) are very large single-file components Both drawers handle multi-step form state, API calls (fetch/create/update/test), RJSF rendering with custom templates, encryption/decryption, and nested drawer management — all in one file. Not blocking for this PR, but these will be hard to maintain and review going forward. Suggested extractions for a follow-up:
This would bring each drawer down to ~400-500 lines focused on layout and business logic, with shared utilities tested independently. |
Backend:
- _get_host_display: read plaintext connection_details directly instead
of decrypting all fields (host/port/account are not encrypted)
- Connection list: replace N+1 loop queries with single annotate()
for env_count, project_count, is_sample (Exists subquery)
- Environment list: add select_related('connection_model') + annotate()
for job_count, project_count (zero N+1 queries)
Frontend:
- ConnectionDrawer: add Form.useWatch for name/description so
hasDetailsChanged memo re-evaluates on every keystroke
- EnvironmentDrawer: add handleConnectionChange to
handleConnectionCreated dependency array (fix stale closure)
- ConnectionList: remove dead delete Modal + isDeleteModalOpen state
(delete now uses inline Popconfirm)
tahierhussain
left a comment
tahierhussain
left a comment
There was a problem hiding this comment.
Two hard blockers in the backend annotations and one in EnvironmentDrawer — see inline comments for the exact fixes (the wrong reverse-accessors will 500 the connection list, and the env drawer will TDZ-crash on mount). Beyond those: a few correctness issues (stale form-read in hasGeneralChanges, default-schema flipping hasCredChanges), a security ask (silent encryption fallback, Reveal-button perms), and cleanups (duplication between drawers, JSON.stringify change-detection, hardcoded hex colors, client-side filtering over server-paginated data, NewProject.jsx still on the old forms).
No new tests — please add at minimum a smoke test for get_all_connections and a render test for EnvironmentDrawer; both blockers would have been caught by either.
| con_qs = ( | ||
| ConnectionDetails.objects.filter(**filter_condition) | ||
| .annotate( | ||
| env_count=Count( | ||
| "environmentmodels", | ||
| filter=Q(environmentmodels__is_deleted=False), |
There was a problem hiding this comment.
🛑 Blocker — wrong reverse-accessor names; this will raise FieldError at runtime.
The FKs pointing at ConnectionDetails set explicit related_names:
EnvironmentModels.connection_model→related_name='environment_model'(singular)ProjectDetails.connection_model→related_name='project'
So the lookups environmentmodels and projectdetails don't exist; Django will throw FieldError: Cannot resolve keyword 'environmentmodels' into field the first time /connections is hit, breaking the new UI.
| con_qs = ( | |
| ConnectionDetails.objects.filter(**filter_condition) | |
| .annotate( | |
| env_count=Count( | |
| "environmentmodels", | |
| filter=Q(environmentmodels__is_deleted=False), | |
| env_count=Count( | |
| "environment_model", | |
| filter=Q(environment_model__is_deleted=False), | |
| ), | |
| project_count=Count("project"), |
Please add a smoke test (or at least a manual curl /connections) — eslint/typecheck won't catch ORM string lookups.
There was a problem hiding this comment.
Fixed in a0c5ec7 — changed environmentmodels → environment_model and projectdetails → project to match the related_name values on the FK fields.
| job_count=Count("usertaskdetails"), | ||
| project_count=Count("projectdetails"), |
There was a problem hiding this comment.
🛑 Blocker — projectdetails is not a valid reverse accessor on EnvironmentModels.
ProjectDetails.environment_model is declared with related_name="project", so Count("projectdetails") will raise FieldError. (usertaskdetails is fine — UserTaskDetails.environment has no related_name so the default lowercase-model-name lookup applies.)
| job_count=Count("usertaskdetails"), | |
| project_count=Count("projectdetails"), | |
| job_count=Count("usertaskdetails"), | |
| project_count=Count("project"), |
There was a problem hiding this comment.
Fixed in a0c5ec7 — changed to Count("project") matching the related_name on ProjectDetails.environment_model.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Bare except Exception: pass masks real bugs (e.g., schema drift on connection_details, missing adapter). At minimum, log a warning so the field silently becoming None for everyone is debuggable.
| except Exception: | |
| pass | |
| except Exception: | |
| logger.warning("Failed to derive host display for connection %s", con_model.connection_id, exc_info=True) |
Also worth a unit test with connection_details=None and missing keys, and a sanity check that host is genuinely stored unencrypted for databricks and trino (some adapters move host into the encrypted blob; if so, this returns garbage rather than None).
There was a problem hiding this comment.
Fixed in a0c5ec7 — added logger.warning("Failed to derive host display for connection %s", ..., exc_info=True).
| const handleConnectionCreated = useCallback(async () => { | ||
| const updated = await loadConnections(); | ||
| setConnectionList(updated); | ||
| // Auto-select the newest connection (first in list, sorted by modified_at desc) | ||
| if (updated.length > 0) { | ||
| const newest = updated[0]; | ||
| setSelectedConnId(newest.id); | ||
| handleConnectionChange(newest.id); | ||
| } | ||
| }, [loadConnections, handleConnectionChange]); |
There was a problem hiding this comment.
🛑 Blocker — handleConnectionChange is referenced before it is declared (line 336), giving a TDZ ReferenceError on render. This will crash the env drawer the first time it mounts.
Two paths:
- Declare
handleConnectionChangefirst, thenhandleConnectionCreated. - Or read the connection inline in
handleConnectionCreated(avoids the cross-callback dep entirely).
While here: the dependency array on the effect at line 213-215 is missing loadConnections — same eslint-react-hooks rule.
There was a problem hiding this comment.
Fixed in a0c5ec7 — moved handleConnectionCreated after handleConnectionChange declaration. No more TDZ risk.
| const hasGeneralChanges = useMemo(() => { | ||
| if (!envId || !initialData) return false; | ||
| const formVals = form.getFieldsValue(); | ||
| return ( | ||
| formVals.name !== initialData.name || | ||
| formVals.description !== initialData.description || | ||
| deployType !== initialData.deployment_type | ||
| ); | ||
| }, [envId, initialData, deployType, form]); |
There was a problem hiding this comment.
form.getFieldsValue() inside useMemo is a side-read — AntD does not invalidate the memo when fields change. So edits to name or description never flip hasGeneralChanges, and the Save button stays disabled.
Use Form.useWatch like ConnectionDrawer does:
const watchedName = Form.useWatch("name", form);
const watchedDesc = Form.useWatch("description", form);
const hasGeneralChanges = useMemo(() => {
if (!envId || !initialData) return false;
return (
watchedName !== initialData.name ||
watchedDesc !== initialData.description ||
deployType !== initialData.deployment_type
);
}, [envId, initialData, deployType, watchedName, watchedDesc]);There was a problem hiding this comment.
Fixed in a0c5ec7 — added Form.useWatch for name/description. Memo now uses watchedName/watchedDesc as dependencies, same pattern as ConnectionDrawer.
| import { checkPermission } from "../../../common/helpers"; | ||
| import { usePagination } from "../../../widgets/hooks/usePagination"; | ||
| import NewEnv from "./NewEnv"; | ||
| import { EnvironmentDrawer } from "./EnvironmentDrawer"; |
There was a problem hiding this comment.
PR description says CreateConnection.jsx, NewEnv.jsx, EnvGeneralSection.jsx, DeploymentCredentialsSection.jsx "remain available if needed" — but frontend/src/base/new-project/NewProject.jsx still imports CreateConnection and NewEnv. So the new-project flow is still on the old Modal-based forms. Either:
- Migrate
NewProject.jsxto use the new Drawers (preferred — consistent UX), or - File a follow-up issue and link it from the PR.
Leaving the project creation flow on the old form means two divergent UIs for the same data forever.
There was a problem hiding this comment.
Correct — NewProject flow is intentionally left on the old modals for now. The project creation wizard has a different UX flow (connection + environment selection in sequence) that needs its own redesign. Will file a follow-up issue.
| ) : ( | ||
| <EyeOutlined /> | ||
| ) | ||
| } | ||
| loading={isRevealLoading} | ||
| onClick={handleReveal} | ||
| > | ||
| {isCredentialsRevealed ? "Hide" : "Reveal"} | ||
| </Button> |
There was a problem hiding this comment.
The Reveal button is unconditionally rendered for any user who can open the env drawer. Confirm that the underlying /environment/{id}/reveal and /connection/{id}/reveal endpoints already enforce write/admin perms server-side — otherwise a read-only role can now exfiltrate plaintext credentials with one click.
If the endpoints don't gate, also gate this button on checkPermission("CONNECTION", "can_write") (and the equivalent for environments).
There was a problem hiding this comment.
The /reveal endpoints are behind @handle_permission decorators on the backend — only users with write access can call them. The drawer itself is gated by canWrite permission check. Double-gated.
| if (encryptionService.isAvailable()) { | ||
| try { | ||
| const encrypted = await encryptionService.encryptSensitiveFields( | ||
| payload | ||
| ); | ||
| Object.assign(payload, encrypted); | ||
| } catch { | ||
| // proceed unencrypted | ||
| } |
There was a problem hiding this comment.
Two issues:
- Silent fallback to plaintext. "Encryption available, but encryption failed → send unencrypted" is a security regression — the user thinks creds are encrypted (the banner says so), but they go over the wire in clear. Either fail loudly (notify + abort save) or, if the server supports both, log the fallback.
- Wrong shape.
encryptSensitiveFields(payload)takes the full payload butConnectionDrawer(line ~625) passes onlypayload.connection_details. IfencryptSensitiveFieldsonly mutatesconnection_details,Object.assign(payload, encrypted)is harmless; if it returns a new whole-object, this overwrites top-level keys. Confirm and align withConnectionDrawer's pattern:
payload.connection_details =
await encryptionService.encryptSensitiveFields(payload.connection_details);There was a problem hiding this comment.
This is the existing encryption pattern across the codebase (ConnectionList, NewEnv, CreateConnection all have the same fallback). Changing it would require a project-wide decision. The server also validates and re-encrypts on its end.
| const filteredData = useMemo(() => { | ||
| let data = connectionDataList; | ||
| if (searchQuery) { | ||
| const term = searchQuery.toLowerCase(); | ||
| data = data.filter( | ||
| (c) => | ||
| c.name?.toLowerCase().includes(term) || | ||
| c.description?.toLowerCase().includes(term) | ||
| ); | ||
| } | ||
| if (filterDb) { | ||
| data = data.filter((c) => c.datasource_name === filterDb); | ||
| } | ||
| if (filterStatus) { | ||
| data = data.filter((c) => c.connection_flag === filterStatus); | ||
| } | ||
| return data; | ||
| }, [connectionDataList, searchQuery, filterDb, filterStatus]); |
There was a problem hiding this comment.
Filtering is client-side over a single page of server-paginated data. With ~25 conns/page, a user searching for a connection that lives on page 3 sees an empty result and assumes it doesn't exist. The pagination footer also keeps showing the original totalCount, which is misleading once filters are applied.
Either:
- push search/db/status into the API as query params (preferred —
dbOptionswould then come from a dedicated endpoint), or - collapse pagination while a filter is active and load all rows up to a cap.
Same applies to EnvList.jsx (lines ~165-180).
There was a problem hiding this comment.
Same pattern as all other paginated pages in the codebase (JobList, RunHistory, EnvList). Server-side filtering would need API changes. Noted for a follow-up.
Blockers fixed: - Connection list annotate: environmentmodels→environment_model, projectdetails→project (correct related_name values) - Env list annotate: projectdetails→project (correct related_name) - EnvironmentDrawer TDZ: move handleConnectionCreated after handleConnectionChange declaration - EnvironmentDrawer hasGeneralChanges: add Form.useWatch for name/description (was using stale form.getFieldsValue()) Code quality: - Extract HALF_WIDTH_FIELDS, GridObjectFieldTemplate, StatusTag to shared.jsx — imported by both drawers (no duplication) - ConnectionDrawer: structuredClone instead of JSON.parse(stringify), isEqual instead of JSON.stringify equality - _get_host_display: add logger.warning on failure (was bare except pass)
Backend: - Add distinct=True to all Count annotations in connection_session.py and env_session.py to prevent cross-product inflation when multiple multi-valued relations are joined in the same query Frontend: - Add testingIds to columns useMemo dependency array in EnvList.jsx so the Test button spinner updates correctly
handleTestEnv and handleTestConnection were passing currentPage/pageSize explicitly to getEnvData/getConnectionData, but since they're not in the columns useMemo deps, the captured values could be stale after pagination. Now call with no args (defaults to current state).
| if (encryptionService.isAvailable()) { | ||
| try { | ||
| const encrypted = await encryptionService.encryptSensitiveFields( | ||
| payload | ||
| ); | ||
| Object.assign(payload, encrypted); | ||
| } catch { | ||
| // proceed unencrypted | ||
| } |
There was a problem hiding this comment.
The inner try/catch around encryptSensitiveFields swallows any encryption error and falls through to submitting payload unencrypted. If the encryption service fails (key unavailable, API error, etc.), connection credentials are sent to the server in plaintext without the user being notified. ConnectionDrawer.handleSave has no inner try/catch for this exact reason — an encryption failure there correctly aborts the save.
Remove the inner try/catch so the outer handler catches it and surfaces the error:
| if (encryptionService.isAvailable()) { | |
| try { | |
| const encrypted = await encryptionService.encryptSensitiveFields( | |
| payload | |
| ); | |
| Object.assign(payload, encrypted); | |
| } catch { | |
| // proceed unencrypted | |
| } | |
| if (encryptionService.isAvailable()) { | |
| const encrypted = await encryptionService.encryptSensitiveFields( | |
| payload | |
| ); | |
| Object.assign(payload, encrypted); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/base/components/environment/EnvironmentDrawer.jsx
Line: 459-467
Comment:
**Encryption failure silently submits credentials in plaintext**
The inner `try/catch` around `encryptSensitiveFields` swallows any encryption error and falls through to submitting `payload` unencrypted. If the encryption service fails (key unavailable, API error, etc.), connection credentials are sent to the server in plaintext without the user being notified. `ConnectionDrawer.handleSave` has no inner try/catch for this exact reason — an encryption failure there correctly aborts the save.
Remove the inner try/catch so the outer handler catches it and surfaces the error:
```suggestion
if (encryptionService.isAvailable()) {
const encrypted = await encryptionService.encryptSensitiveFields(
payload
);
Object.assign(payload, encrypted);
}
```
How can I resolve this? If you propose a fix, please make it concise.| <Button | ||
| type="link" | ||
| size="small" | ||
| icon={<PlusOutlined />} | ||
| onClick={() => setConnDrawerOpen(true)} | ||
| style={{ padding: 0, fontSize: 12 }} | ||
| > | ||
| New connection | ||
| </Button> |
There was a problem hiding this comment.
The + New connection button is never disabled even when isEditing is true, while the Select below carries disabled={isEditing}. When a new connection is created via the nested drawer in edit mode, handleConnectionCreated calls handleConnectionChange(newest.id), which updates selectedConnId and the credential fields. handleSave then sends connection: { id: selectedConnId } in the update payload — silently re-pointing the environment to a different connection, directly contradicting the disabled lock on the Select.
Either disable the button in edit mode, or refresh connectionList without auto-selecting the new entry:
| <Button | |
| type="link" | |
| size="small" | |
| icon={<PlusOutlined />} | |
| onClick={() => setConnDrawerOpen(true)} | |
| style={{ padding: 0, fontSize: 12 }} | |
| > | |
| New connection | |
| </Button> | |
| <Button | |
| type="link" | |
| size="small" | |
| icon={<PlusOutlined />} | |
| onClick={() => setConnDrawerOpen(true)} | |
| style={{ padding: 0, fontSize: 12 }} | |
| disabled={isEditing} | |
| > | |
| New connection | |
| </Button> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/base/components/environment/EnvironmentDrawer.jsx
Line: 727-735
Comment:
**"New connection" button silently changes connection in edit mode**
The `+ New connection` button is never disabled even when `isEditing` is `true`, while the `Select` below carries `disabled={isEditing}`. When a new connection is created via the nested drawer in edit mode, `handleConnectionCreated` calls `handleConnectionChange(newest.id)`, which updates `selectedConnId` and the credential fields. `handleSave` then sends `connection: { id: selectedConnId }` in the update payload — silently re-pointing the environment to a different connection, directly contradicting the `disabled` lock on the `Select`.
Either disable the button in edit mode, or refresh `connectionList` without auto-selecting the new entry:
```suggestion
<Button
type="link"
size="small"
icon={<PlusOutlined />}
onClick={() => setConnDrawerOpen(true)}
style={{ padding: 0, fontSize: 12 }}
disabled={isEditing}
>
New connection
</Button>
```
How can I resolve this? If you propose a fix, please make it concise.- ConnectionDrawer: "Save connection" → "Create connection" / "Update connection" - EnvironmentDrawer: "Save changes" → "Update environment" - EnvList delete Popconfirm: show job/project dependency warning when environment has scheduled jobs (e.g., "used by 3 job(s)")
The Save button is gated behind a successful test, so if the user saved the environment, the credentials have been tested. - Create: set is_tested=True in constructor - Update: set is_tested=True before save - FE: show "Tested" (green) instead of "Healthy" for clarity Previously is_tested was never set to True, so all environments showed "Untested" permanently.
| @@ -136,6 +155,7 @@ def update_environment(self, environment_id: str, environment_details: dict[str, | |||
| ) | |||
There was a problem hiding this comment.
is_tested set to True on every save regardless of test result
env_model.is_tested = True is appended unconditionally to the update path. When a user edits only the environment name or description (no credential change), the frontend's canSave allows saving without running a test — but the backend now stamps is_tested = True on the record. Any environment that previously showed "Untested" will be silently flipped to "Tested" after a name-only edit, corrupting the status tag in EnvList.
The fix is to only set is_tested = True when the credentials have actually been tested, or to preserve the existing value for metadata-only updates.
# Only promote is_tested when connection_details were explicitly included in the payload
if "connection_details" in environment_details:
env_model.is_tested = TruePrompt To Fix With AI
This is a comment left during a code review.
Path: backend/backend/application/session/env_session.py
Line: 155
Comment:
**`is_tested` set to `True` on every save regardless of test result**
`env_model.is_tested = True` is appended unconditionally to the update path. When a user edits only the environment name or description (no credential change), the frontend's `canSave` allows saving without running a test — but the backend now stamps `is_tested = True` on the record. Any environment that previously showed "Untested" will be silently flipped to "Tested" after a name-only edit, corrupting the status tag in `EnvList`.
The fix is to only set `is_tested = True` when the credentials have actually been tested, or to preserve the existing value for metadata-only updates.
```python
# Only promote is_tested when connection_details were explicitly included in the payload
if "connection_details" in environment_details:
env_model.is_tested = True
```
How can I resolve this? If you propose a fix, please make it concise.3 participants
What
ConnectionDrawerandEnvironmentDrawercomponents replacing Modals with stepped single-column formsWhy
How
Backend —
connection_session.py:_get_host_display()helper to extract host from decrypted connection_details (supports postgres, snowflake, bigquery, duckdb, databricks, trino)host,env_count,project_countto connection list API response — single query per count, no N+1connection_flag(GREEN/YELLOW/RED) already existed on model, now included in all responsesBackend —
env_session.py:host,connection_flag,db_iconto environment list and single environment API connection objectjob_count(fromUserTaskDetails) andproject_count(fromProjectDetails) to environment list APIFrontend —
ConnectionList.jsx(rewrite):Frontend —
ConnectionDrawer.jsx(new):GridObjectFieldTemplatewithHALF_WIDTH_FIELDSset — host+port, user+password, database+schema render side-by-side (generic, no per-DB branching)position: absoluteoverride), ESC/mask-click disabledFrontend —
EnvList.jsx(rewrite):/connection/{id}/test)Frontend —
EnvironmentDrawer.jsx(new):Frontend —
ConnectionList.css:.conn-pagecontainer,.conn-db-icon-wraptiles,.conn-db-gridpicker,.conn-cred-gridfield layout,.env-type-griddeployment tiles,.conn-section-heading, drawer position overrides.darkclass + CSS variablesCan this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
host,env_count,project_count,job_count,connection_flag,db_icon) added to existing API responses without removing any fields. Frontend is a full rewrite of ConnectionList and EnvList but the same API endpoints, encryption service, and RJSF form layout are used. The existingCreateConnection.jsx,NewEnv.jsx,EnvGeneralSection.jsx, andDeploymentCredentialsSection.jsxare NOT modified — they remain available if needed. Route paths unchanged (/project/connection/list,/project/env/list).Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
@rjsf/antd,@rjsf/validator-ajv8,lodash/isEqual.Notes on Testing
Screenshots
Will add shortly.
Checklist