refactor: simplify enrichment column name logic#373
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors enrichment column naming to treat the enrichment name as the canonical (prefixed) column name everywhere, removing the previous “externalName vs name” split and updating server, jobs, adaptors, and tests accordingly.
Changes:
- Replace
columns[].def.externalNamewithcolumns[].def.name(viaColumnDef) across enrichment pipelines and adaptor update paths. - Remove
enrichmentColumnName()and update server/router/job APIs to usecolumnNamesconsistently. - Update UI and tests to create/store enrichment names already prefixed with
ENRICHMENT_COLUMN_PREFIX(e.g."Mapped: …").
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/server/adaptors/mailchimp.test.ts | Update enriched record column defs to use def.name instead of def.externalName. |
| tests/unit/server/adaptors/googlesheets.test.ts | Update enriched record column defs to use def.name. |
| tests/unit/server/adaptors/airtable.test.ts | Update enriched record column defs to use def.name. |
| tests/unit/server/adaptors/actionnetwork.test.ts | Update enriched record column defs to use def.name. |
| tests/feature/enrichDataSource.test.ts | Update enrichment config names to be prefixed ("Mapped: …") and job args to columnNames. |
| src/utils/dataRecord.ts | Remove enrichmentColumnName() helper and its unused import. |
| src/server/trpc/routers/dataSource.ts | Stop using enrichmentColumnName(), rename delete API input to columnNames, and propagate to cleanup job. |
| src/server/repositories/DataRecord.ts | Write enrichment values to data_record.json using column.def.name key. |
| src/server/repositories/Area.ts | Change spatial predicate in findAreasByPoint from ST_DWithin to ST_Covers. |
| src/server/models/DataRecord.ts | Update EnrichedRecord column def type to ColumnDef. |
| src/server/mapping/enrich.ts | Emit enriched columns with def.name = enrichment.name; rename cleanup arg to columnNames. |
| src/server/jobs/removeEnrichmentColumns.ts | Rename job payload field to columnNames and use it for external deletion + JSON cleanup. |
| src/server/jobs/enrichDataSource.ts | Collect enriched column defs keyed by col.def.name. |
| src/server/jobs/enrichDataRecords.ts | Collect enriched column defs keyed by col.def.name. |
| src/server/adaptors/mailchimp.ts | Use column.def.name for merge field naming/lookup. |
| src/server/adaptors/googlesheets.ts | Use column.def.name when adding headers and computing update indices. |
| src/server/adaptors/airtable.ts | Use column.def.name when building Airtable fields and new field set. |
| src/server/adaptors/actionnetwork.ts | Use column.def.name when building customFields payload. |
| src/app/api/data-sources/[id]/enriched-csv/route.ts | Use enrichment.name directly as enriched CSV column name. |
| src/app/(private)/data-sources/components/EnrichmentColumnDialog.tsx | Prefix new enrichment names at creation time using ENRICHMENT_COLUMN_PREFIX. |
| src/app/(private)/data-sources/[id]/DataSourceEnrichmentDashboard.tsx | Stop mapping enrichment names via enrichmentColumnName(); update delete call arg name; add invalidations on ImportComplete. |
Comments suppressed due to low confidence (2)
src/server/mapping/enrich.ts:202
removeEnrichmentColumnsFromDataSourcenow removes enrichments by exact string match one.name. If callers pass prefixed column names (e.g."Mapped: Region") but the stored enrichments were created with unprefixed names in older data, this will leave stale enrichment metadata behind while removing the columnDefs. Consider normalizing both the storedenrichments[].namevalues and thecolumnNamesargument to a single canonical form before filtering.
export const removeEnrichmentColumnsFromDataSource = async (
dataSourceId: string,
columnNames: string[],
) => {
if (columnNames.length === 0) return;
const dataSource = await findDataSourceById(dataSourceId);
if (!dataSource) return;
const namesToRemove = new Set(columnNames);
const remainingEnrichments = (dataSource.enrichments ?? []).filter(
(e) => !namesToRemove.has(e.name),
);
const remainingColumnDefs = (dataSource.columnDefs ?? []).filter(
(col) => !namesToRemove.has(col.name),
);
src/app/(private)/data-sources/components/EnrichmentColumnDialog.tsx:180
newEnrichmentsalways prefixes the user-enterednamewithENRICHMENT_COLUMN_PREFIX. If the user types/pastes a name that already includes the prefix, this will produce double-prefixed column names (e.g."Mapped: Mapped: Region"). Consider normalizing the input (only add the prefix if missing, or strip it from the editable field and render the prefix separately) so the stored enrichment/column name is canonical.
const newEnrichments = useMemo(() => {
if (columnType === "geographic") {
const result = enrichmentSchema.safeParse({
name: `${ENRICHMENT_COLUMN_PREFIX}${name}`,
sourceType: EnrichmentSourceType.Area,
areaSetCode,
areaProperty,
});
return result.success ? [result.data] : null;
}
if (columnType === "data") {
const enrichments = selectedColumns.map((col) =>
enrichmentSchema.safeParse({
name: `${ENRICHMENT_COLUMN_PREFIX}${selectedColumns.length === 1 ? name : col}`,
sourceType: EnrichmentSourceType.DataSource,
dataSourceId: selectedDataSourceId,
dataSourceColumn: col,
}),
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
171
to
176
| const existingColumnNames = new Set( | ||
| (dataSource.columnDefs ?? []).map((col) => col.name), | ||
| ); | ||
| const newEnrichments = dataSource.enrichments.filter( | ||
| (e) => !existingColumnNames.has(enrichmentColumnName(e.name)), | ||
| (e) => !existingColumnNames.has(e.name), | ||
| ); |
Comment on lines
471
to
480
| deleteEnrichmentColumns: dataSourceOwnerProcedure | ||
| .input(z.object({ externalColumnNames: z.array(z.string()) })) | ||
| .input(z.object({ columnNames: z.array(z.string()) })) | ||
| .mutation(async ({ ctx, input }) => { | ||
| for (const externalColumnName of input.externalColumnNames) { | ||
| if (!externalColumnName.startsWith(ENRICHMENT_COLUMN_PREFIX)) { | ||
| for (const columnName of input.columnNames) { | ||
| if (!columnName.startsWith(ENRICHMENT_COLUMN_PREFIX)) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `Column "${externalColumnName}" is not an enrichment column`, | ||
| message: `Column "${columnName}" is not an enrichment column`, | ||
| }); | ||
| } |
Comment on lines
10
to
17
| if ( | ||
| !args || | ||
| !("dataSourceId" in args) || | ||
| !("externalColumnNames" in args) || | ||
| !Array.isArray(args.externalColumnNames) | ||
| !("columnNames" in args) || | ||
| !Array.isArray(args.columnNames) | ||
| ) { | ||
| return false; | ||
| } |
Comment on lines
77
to
107
| // Uses the generated `geom` column for 10x performance improvement, | ||
| // at the loss of some (potentially negligible) accuracy. | ||
| export async function findAreasByPoint({ | ||
| point, | ||
| excludeAreaSetCode, | ||
| includeAreaSetCode, | ||
| }: { | ||
| point: Point | null; | ||
| excludeAreaSetCode?: AreaSetCode | null | undefined; | ||
| includeAreaSetCode?: AreaSetCode | null | undefined; | ||
| }): Promise<AreaWithAreaSetCode[]> { | ||
| if (!point) { | ||
| return []; | ||
| } | ||
| const pointGeoJSON = JSON.stringify({ | ||
| type: "Point", | ||
| coordinates: [point.lng, point.lat], | ||
| }); | ||
| // Use the read replica for this expensive read query | ||
| let query = dbRead | ||
| .selectFrom("area") | ||
| .innerJoin("areaSet", "area.areaSetId", "areaSet.id"); | ||
| if (excludeAreaSetCode) { | ||
| query = query.where("areaSet.code", "!=", excludeAreaSetCode); | ||
| } | ||
| if (includeAreaSetCode) { | ||
| query = query.where("areaSet.code", "=", includeAreaSetCode); | ||
| } | ||
| return query | ||
| .where( | ||
| sql<boolean>`ST_DWithin(geom, ST_GeomFromGeoJson(${pointGeoJSON}), 0.01)`, | ||
| ) | ||
| .where(sql<boolean>`ST_Covers(geom, ST_GeomFromGeoJson(${pointGeoJSON}))`) | ||
| .select([ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.