fix: datastore integration text taxonomy [ENG-3209]#7943
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
33d78b6 to
fc38e0c
Compare
chore: update changelog fix: missing commit chore: linting
fc38e0c to
166cbac
Compare
There was a problem hiding this comment.
Code Review: PR #7943 — Datastore integration text taxonomy
The change is clean and correctly addresses the problem: the UI was hardcoded to BigQuery terminology ("projects") even for non-BigQuery integrations. The as const narrowing on DATASTORE_CONTENT_TAXONOMY produces exact literal tuple types, and the prop typing with readonly [string, string] is a good fit.
Summary of findings
Default value bias (minor) — ConfigureMonitorDatabasesForm defaults to ["project", "projects"] which is BigQuery-specific. Since ConfigureMonitorModal always supplies the prop now, this only affects standalone usage, but it's a surprising default. Prefer a neutral fallback or make the prop required. See inline comment.
Scalability of the BIGQUERY check (minor) — The connection_type === ConnectionType.BIGQUERY conditional works for the current two-way split but may need to grow as more integration types are onboarded (Snowflake, Redshift, MongoDB, etc. all could have distinct preferred terminology). Keying DATASTORE_CONTENT_TAXONOMY directly off ConnectionType values would keep future additions in one place. See inline comment with a concrete pattern.
No tests — Neither component has existing test coverage, so there's no regression, but adding a basic snapshot or render test for the new contentTaxonomy prop (verifying both "project" and "database" variants render the correct strings) would guard against future copy changes.
What looks good
as const+readonly [string, string]is properly type-safe.- Template literals are consistently applied across all three text sites (header, tooltip, and save-disabled tooltip).
- The old
TOOLTIP_COPYconstant (hardcoded "project") is cleanly removed — no dead code left behind. - Changelog entry is present.
Overall this is a straightforward, low-risk fix. The two inline comments are suggestions, not blockers.
🔬 Codegraph: connected (46795 nodes)
💡 Write /code-review in a comment to re-run this review.
Ticket ENG-3209
Description Of Changes
Changes the taxonomy of the datastore monitor content based on the type of monitor being created/edited.
Code Changes
Steps to Confirm
projectsdatabasesPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works