feat(connections): colored tags and environment classification with production safety warning - #473
Conversation
Add ConnectionTag {id, name, color} stored in connections.json alongside
groups, with tag_ids on each connection. New connection_tags module with
list/create/update/delete_connection_tag and set_connection_tags commands:
names unique case-insensitively, hex colors validated, deleting a tag
detaches it everywhere. Tags ride along in export/import/backups and
survive update/duplicate.
UI: tag picker in the connection modal appearance tab (inline creation
with the shared accent palette, manage mode to rename/recolor/delete),
colored chips on connection cards and list rows, and tag names take part
in the connection search filter.
First slice of TabularisDB#472; environments and the production warning come next.
…p name length set_connection_tags now drops unknown ids instead of failing, so a connection imported with orphaned tag references can still be saved. Imports unify tags that share a name (case-insensitive) onto the existing tag and remap the imported connections tag_ids, so re-creating prod on another machine no longer produces duplicates. Tag names are capped at 32 characters, enforced in the backend and mirrored by maxLength on inputs.
…warning Add an optional environment field (development/staging/production) on each connection, picked from the connection modal title bar and validated in the backend; it survives duplicate and import. Production connections get an unmistakable identity: a PROD badge on cards and list rows, a permanent red banner while the active connection is production, and a red ring on open sidebar entries. Writes against production ask for confirmation first. Detection reuses the sqlAnalysis infrastructure: only SELECT/SHOW/DESCRIBE/PRAGMA/EXPLAIN-of-a- SELECT statements are considered read-only; data-modifying CTEs, EXPLAIN ANALYZE of writes and unknown statement types (CALL, SET, ...) all prompt. Guarded paths: editor single and multi-statement runs, staged grid-edit commits, immediate cell edits, row insertion and notebook cells, stacking with the existing dangerous-query guard. The dialog previews the SQL and offers a per-connection snooze that lasts until the app restarts. Completes TabularisDB#472 together with the tags feature.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| let mut remap = std::collections::HashMap::new(); | ||
| for tag in imported { | ||
| if let Some(same_id) = existing.iter_mut().find(|t| t.id == tag.id) { | ||
| *same_id = tag; |
There was a problem hiding this comment.
CRITICAL: Import merge can create duplicate tag names
merge_imported_tags overwrites an existing tag by same-id (*same_id = tag) without checking whether the imported tag's new name collides with another existing tag. If existing tags are ["Prod", "Dev"] and an import renames "Prod" to "Dev", the result is two tags both named "Dev", breaking the unique-name invariant enforced by create_tag_impl/update_tag_impl. Consider validating the name collision before overwriting, or unifying onto the colliding tag instead.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // Merge connections and handle passwords | ||
| for mut new_conn in payload.connections { | ||
| for mut new_conn in payload_connections { |
There was a problem hiding this comment.
WARNING: Imported connections bypass environment validation
apply_export_payload iterates over imported connections but never calls validate_environment on new_conn.environment. An exported payload (from an older version, a foreign app, or a manually crafted file) can contain arbitrary environment strings that bypass the validation enforced by save_connection and update_connection. Consider validating each imported connection's environment before saving.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| detect_json_in_text_columns: original.detect_json_in_text_columns, | ||
| appearance: new_appearance, | ||
| tag_ids: original.tag_ids.clone(), | ||
| environment: original.environment.clone(), |
There was a problem hiding this comment.
WARNING: Duplicate propagates invalid environment without validation
duplicate_connection copies original.environment.clone() directly into the new connection. If the original holds an invalid environment value (e.g. introduced by file corruption or an unvalidated import), the invalid value propagates silently. Consider passing it through validate_environment.
| environment: original.environment.clone(), | |
| environment: validate_environment(original.environment.clone())?, |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (!targetTab) return; | ||
|
|
||
| if (!(await guardDangerousQuery(queries))) return; | ||
| if (!(await guardProductionWrite(activeConnectionId, queries.join(";\n")))) { |
There was a problem hiding this comment.
WARNING: Missing guardProductionWrite in useCallback deps
guardProductionWrite is invoked inside runMultipleQueries but is absent from the dependency array. This risks a stale closure if the guard reference changes (e.g. when connections updates in useProductionGuard). The runQuery callback correctly includes it; this one should too.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return; | ||
|
|
||
| // Production safety: grid edits are writes, confirm before committing. | ||
| if (!(await guardProductionWrite(activeConnectionId))) return; |
There was a problem hiding this comment.
WARNING: Missing guardProductionWrite in useCallback deps
guardProductionWrite is awaited inside handleSubmitChanges but is omitted from the useCallback dependency array. This means a stale guard reference can be captured, potentially using outdated connection state. Consider adding it to the deps list.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| setError(null); | ||
| try { | ||
| const tag = await createTag(newName.trim(), newColor); | ||
| onChange([...selectedIds, tag.id]); |
There was a problem hiding this comment.
WARNING: Stale closure can overwrite newer tag selections
handleCreate captures selectedIds from its closure at render time. If the user toggles other tag chips while createTag is in flight, the newer selections are overwritten when creation completes because the stale closure calls onChange([...selectedIds, tag.id]). Consider accepting a functional updater or reading the latest selected IDs via a ref.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge This is an incremental review since commit Previous findings re-verifiedAll six previously raised findings are resolved at current HEAD and remain unchanged through the merge:
Repair commit (
|
| Severity | Count |
|---|---|
| CRITICAL | 1 |
| WARNING | 5 |
| SUGGESTION | 0 |
Issue Details (click to expand)
CRITICAL
| File | Line | Issue |
|---|---|---|
src-tauri/src/connection_tags.rs |
150 | Import merge can create duplicate tag names |
WARNING
| File | Line | Issue |
|---|---|---|
src-tauri/src/commands.rs |
5059 | Imported connections bypass environment validation |
src-tauri/src/commands.rs |
1076 | Duplicate propagates invalid environment without validation |
src/pages/Editor.tsx |
885 | Missing guardProductionWrite in useCallback deps |
src/pages/Editor.tsx |
2240 | Missing guardProductionWrite in useCallback deps |
src/components/modals/NewConnectionModal/TagSelector.tsx |
78 | Stale closure can overwrite newer tag selections |
Additional Finding (summary-only)
| File | Issue |
|---|---|
src-tauri/src/commands.rs |
apply_export_payload overwrites existing connections with *existing = new_conn. Older export backups deserialize missing tag_ids and environment to None via serde(default), so importing an older backup wipes any existing tags/environment on target connections. Consider merging fields instead of full overwrite for backward compatibility. |
Files Reviewed (38 files)
src-tauri/src/commands.rs- 2 issuessrc-tauri/src/connection_tags.rs- 1 issuesrc/pages/Editor.tsx- 2 issuessrc/components/modals/NewConnectionModal/TagSelector.tsx- 1 issuesrc-tauri/src/models.rssrc-tauri/src/persistence.rssrc-tauri/src/lib.rssrc-tauri/src/connection_cache_tests.rssrc-tauri/src/connection_import/analyzer.rssrc-tauri/src/connection_import/convert.rssrc-tauri/src/connection_import/tabularis.rssrc-tauri/src/export_import_tests.rssrc/components/connections/ConnectionCard.tsxsrc/components/connections/ConnectionListItem.tsxsrc/components/connections/EnvironmentBadge.tsxsrc/components/connections/TagChips.tsxsrc/components/layout/MainLayout.tsxsrc/components/layout/ProductionBanner.tsxsrc/components/layout/sidebar/OpenConnectionItem.tsxsrc/components/modals/NewConnectionModal.tsxsrc/components/modals/NewConnectionModal/AppearanceSection.tsxsrc/components/modals/NewRowModal.tsxsrc/components/notebook/NotebookView.tsxsrc/components/ui/DataGrid.tsxsrc/contexts/DatabaseContext.tssrc/contexts/ProductionGuardContext.tsxsrc/hooks/useConnectionTags.tssrc/main.tsxsrc/pages/Connections.tsxsrc/types/tags.tssrc/utils/environment.tssrc/utils/sqlAnalysis.tssrc/utils/sqlAnalysis.test.tssrc/i18n/locales/de.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/tl.jsonsrc/i18n/locales/zh.json
Reviewed by laguna-s-2.1:free · Input: 456K · Output: 77.4K · Cached: 2.1M
- unify tag rename collisions on import instead of duplicating names, remapping existing connections too - normalize environment on import and duplicate (invalid -> unclassified) - add guardProductionWrite to useCallback deps in Editor - read latest tag selection via ref in async TagSelector handlers - move PALETTE and useProductionGuard to dedicated files (react-refresh) - load tags via promise chain instead of setState in effect body
|
@pokertour can you resolve merge conflicts? |
|
Thanks @pokertour for this contribution! This looks like a great starting point that we can expand on in future PRs. I pushed a small commit on top to repair the merge with main (two build errors, a few test mocks, and the ProductionBanner render that got lost in the conflict resolution). Some ideas for follow-up PRs building on this foundation:
Merging, thanks again! |
2 participants
Closes #472
What
Two complementary features for organizing connections and protecting production data, following the DBeaver model:
Free-form colored tags
ConnectionTag {id, name, color}stored inconnections.jsonalongside groups; connections carrytag_ids.tag_idsremapped), so re-creating "prod" on another machine never duplicates. Orphaned tag ids are tolerated.Environment classification + production warning
environmentfield per connection (development/staging/production), picked in the modal title bar, validated in the backend, preserved across duplicate/import.EXPLAIN ANALYZE <write>(which executes on Postgres) and unknown statement types (CALL, SET, …) all prompt. Guarded paths: editor runs (single + multi-statement), staged grid-edit commits, immediate cell edits, row insertion, notebook cells. Stacks with the existing dangerous-query guard.Tests
isReadOnlyQuery; full suite green (154 files / 2841 tests).Would love some tests and feedback, especially on database other than sqlite and mariadb.