refactor: location #3289

AmarTrebinjac · 2025-11-18T23:33:49Z

After a lot of back and forth we decided that we are going to use Mapbox API instead of seeded data, due to the data we got being very low quality.

I removed the timezone and ranking for two reasons:

RFC doesn't mention how or if we are using these at all
Mapbox API doesn't provide timezones, and does its own ranking when searching, so redundant for us.

AmarTrebinjac · 2025-11-18T23:34:29Z

src/entity/dataset/DatasetLocation.ts

-  ranking: number;
-
-  @Column({ type: 'text', nullable: true })
+  @Index('IDX_dataset_location_externalId')


Added index since we are now querying it on profile save

pulumi · 2025-11-18T23:36:49Z

🍹 The Update (preview) for dailydotdev/api/prod (at f1f1732) was successful.

Resource Changes

    Name                                                   Type                           Operation
~   vpc-native-validate-active-users-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-generic-referral-reminder-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-personalized-digest-deployment              kubernetes:apps/v1:Deployment  update
~   vpc-native-update-source-tag-view-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-clickhouse-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-update-trending-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-update-tag-recommendations-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-stale-user-transactions-cron          kubernetes:batch/v1:CronJob    update
-   vpc-native-api-db-migration-ce7d5dd7                   kubernetes:batch/v1:Job        delete
+   vpc-native-api-db-migration-cfb95f72                   kubernetes:batch/v1:Job        create
~   vpc-native-update-tags-str-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-hourly-notification-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-check-analytics-report-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-daily-digest-cron                           kubernetes:batch/v1:CronJob    update
~   vpc-native-private-deployment                          kubernetes:apps/v1:Deployment  update
~   vpc-native-clean-zombie-images-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-deployment                                  kubernetes:apps/v1:Deployment  update
~   vpc-native-clean-gifted-plus-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-update-source-public-threshold-cron         kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-users-cron                     kubernetes:batch/v1:CronJob    update
-   vpc-native-api-clickhouse-migration-ce7d5dd7           kubernetes:batch/v1:Job        delete
~   vpc-native-calculate-top-readers-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-user-companies-cron            kubernetes:batch/v1:CronJob    update
~   vpc-native-generate-search-invites-cron                kubernetes:batch/v1:CronJob    update
~   vpc-native-temporal-deployment                         kubernetes:apps/v1:Deployment  update
~   vpc-native-sync-subscription-with-cio-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-update-highlighted-views-cron               kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-history-day-clickhouse-cron  kubernetes:batch/v1:CronJob    update
+   vpc-native-api-clickhouse-migration-cfb95f72           kubernetes:batch/v1:Job        create
~   vpc-native-update-views-cron                           kubernetes:batch/v1:CronJob    update
~   vpc-native-ws-deployment                               kubernetes:apps/v1:Deployment  update
~   vpc-native-personalized-digest-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-bg-deployment                               kubernetes:apps/v1:Deployment  update
~   vpc-native-update-current-streak-cron                  kubernetes:batch/v1:CronJob    update
+-  vpc-native-k8s-secret                                  kubernetes:core/v1:Secret      create-replacement

AmarTrebinjac · 2025-11-19T09:53:24Z

src/entity/dataset/utils.ts

+  let mapboxGeocodingUrl = `${process.env.MAPBOX_GEOCODING_URL}?q=${encodeURIComponent(externalId)}&access_token=${process.env.MAPBOX_ACCESS_TOKEN}`;
+
+  if (process.env.NODE_ENV === 'production') {
+    mapboxGeocodingUrl += '&permanent=true';


We need to use this prop to be allowed to save locations locally.

feel free to add this as a comment, it will be forgotten

could also use URL instead of string concat for all the other params, much less error prone

AmarTrebinjac · 2025-11-19T09:55:53Z

src/schema/autocompletes.ts

+        const response = await fetch(mapboxUrl);
+
+        if (!response.ok) {
+          return [];


Not sure if we wanna throw an error here or just return empty? Cause if the user starts typing again it will trigger the endpoint again, and then we might have an OK response.

AmarTrebinjac · 2025-11-19T11:44:56Z

src/entity/dataset/utils.ts

+  const response = await fetch(mapboxGeocodingUrl);
+
+  if (!response.ok) {
+    throw new Error('Mapbox API error');


Not sure what type of error would be appropriate here?

Error is fine, it will be 5xx, but maybe you want to add alerts if you think its necessary we know when it fails.

Also with 3party integrations we usually use Garmr for automatic retries, failover and so on so ideally you add that. Garmr will automatically notifiy on the channel.

I thought this added alerts:

ctx.log.error( { error }, 'Failed to fetch location autocomplete from Mapbox', );

Is there something specific we need to do to hook it up to our slack channel?

if you hook up garmr there are alerts for it automatically so you don't need to do anything

as for log, it does not send all logs to slack, that would be so spammy 😆 you have to set manual alert for it, I can show you, but as said, garmr should be enough here without manual ctx.log.error call

capJavert

Some comments, blocking stuff is around adding Garmr when interacting with mapbox. Since with Garmr you get reporting and better error and retries handling for free.

capJavert · 2025-11-19T11:49:19Z

bin/import.ts

    return await importEntity(con, params.entity);
  }

-  await importEntity(con, 'DatasetLocation');


also remove seeds then?

capJavert · 2025-11-19T11:50:29Z

src/common/schema/profile.ts

+    externalLocationId: z.preprocess(
      (val) => (val === '' ? null : val),
-      z.string().uuid().nullable().optional().default(null),
+      z.string().nullable().optional().default(null),


there is nullish which is shorthand for nullable and optional

capJavert · 2025-11-19T11:52:58Z

src/entity/dataset/utils.ts

+import type { DataSource } from 'typeorm/data-source';
+import { DatasetLocation } from './DatasetLocation';
+
+export interface MapboxContext {


let's move this to common or integrations, like we have for other 3party stuff, entity folder is plagued with dependency cycles so it would ideally be as clean as possible to avoid new cycles imports

capJavert · 2025-11-19T11:53:26Z

src/entity/dataset/utils.ts

+  let mapboxGeocodingUrl = `${process.env.MAPBOX_GEOCODING_URL}?q=${encodeURIComponent(externalId)}&access_token=${process.env.MAPBOX_ACCESS_TOKEN}`;
+
+  if (process.env.NODE_ENV === 'production') {
+    mapboxGeocodingUrl += '&permanent=true';


feel free to add this as a comment, it will be forgotten

capJavert · 2025-11-19T11:53:47Z

src/entity/dataset/utils.ts

+  let mapboxGeocodingUrl = `${process.env.MAPBOX_GEOCODING_URL}?q=${encodeURIComponent(externalId)}&access_token=${process.env.MAPBOX_ACCESS_TOKEN}`;
+
+  if (process.env.NODE_ENV === 'production') {
+    mapboxGeocodingUrl += '&permanent=true';


could also use URL instead of string concat for all the other params, much less error prone

capJavert · 2025-11-19T11:54:55Z

src/entity/dataset/utils.ts

+  const response = await fetch(mapboxGeocodingUrl);
+
+  if (!response.ok) {
+    throw new Error('Mapbox API error');


Error is fine, it will be 5xx, but maybe you want to add alerts if you think its necessary we know when it fails.

Also with 3party integrations we usually use Garmr for automatic retries, failover and so on so ideally you add that. Garmr will automatically notifiy on the channel.

capJavert · 2025-11-19T11:57:18Z

src/schema/users.ts

      const readmeHtml = markdown.render(data.readme || '');

      try {
+        const formProps = excludeProperties(data, ['externalLocationId']);


We had discussion for excludeProperties multiple times before and yes, pro is you can just exclude only one field you need, but ideally I would rather specifically do delete data.externalLocationId here or remove it inside externalLocationId, I don't see the point of this helper personally

Fair enough. Thought I'd use it since we already have it, but I can just delete the prop as well. I'm not very opinionated 😄

its just bad in every way, performance wise as well, excludeProperties will iterate over all properties of an object to find one to delete, where delete will just do it 😆

The one in boot is also there for legacy reasons :hidethepain:

capJavert · 2025-11-19T16:39:02Z

src/integrations/mapbox/clients.ts

+  },
+  retryOpts: {
+    maxAttempts: 3,
+    backoff: 1000,


This will wait for 1000ms after each retry so if we hit rate limit and it keeps failing it will delay any request that calls this for 3+ seconds?

Just making sure its what you want?

refactor: location

5fff087

AmarTrebinjac commented Nov 18, 2025

View reviewed changes

return empty if response not ok

008aab0

AmarTrebinjac added 3 commits November 19, 2025 00:38

delete seed script

eb4f732

update work experience

457dcd8

update tests

7887794

AmarTrebinjac commented Nov 19, 2025

View reviewed changes

AmarTrebinjac added 5 commits November 19, 2025 11:02

update syntax

11e8c16

return external id

ba5240c

move excludeProperties to utils to prevent circular dependency

d8e906d

update imports

71250b9

update boot test

0c76467

AmarTrebinjac marked this pull request as ready for review November 19, 2025 11:44

AmarTrebinjac requested review from a team and capJavert as code owners November 19, 2025 11:44

AmarTrebinjac requested review from omBratteng and rebelchris November 19, 2025 11:44

AmarTrebinjac commented Nov 19, 2025

View reviewed changes

capJavert requested changes Nov 19, 2025

View reviewed changes

AmarTrebinjac added 2 commits November 19, 2025 14:58

add garmr

6d4ef0c

undo excludeProperties, update schema

462035c

AmarTrebinjac requested a review from capJavert November 19, 2025 14:19

AmarTrebinjac and others added 5 commits November 19, 2025 15:20

update comment

7038121

Merge branch 'main' into ENG-62

d0e1c85

delete seed

72c7ab1

Merge branch 'main' into ENG-62

9d2f48e

remove excessive logging

9907fa8

capJavert approved these changes Nov 19, 2025

View reviewed changes

AmarTrebinjac added 3 commits November 19, 2025 17:41

add mapbox env vars

102ca0b

reduce backoff

e5dab76

pulumi

f1f1732

AmarTrebinjac merged commit 42f7535 into main Nov 19, 2025
9 checks passed

AmarTrebinjac deleted the ENG-62 branch November 19, 2025 17:49

refactor: location #3289

refactor: location #3289

Uh oh!

Conversation

AmarTrebinjac commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pulumi bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Resource Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capJavert Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capJavert left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capJavert Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AmarTrebinjac commented Nov 18, 2025 •

edited

Loading

pulumi bot commented Nov 18, 2025 •

edited

Loading

capJavert Nov 19, 2025 •

edited

Loading

capJavert left a comment •

edited

Loading

capJavert Nov 19, 2025 •

edited

Loading