Skip to content

feat: expose post highlight#3907

Merged
capJavert merged 3 commits into
mainfrom
expose-post-highlight
May 25, 2026
Merged

feat: expose post highlight#3907
capJavert merged 3 commits into
mainfrom
expose-post-highlight

Conversation

@capJavert
Copy link
Copy Markdown
Contributor

No description provided.

@capJavert capJavert self-assigned this May 25, 2026
@pulumi
Copy link
Copy Markdown

pulumi Bot commented May 25, 2026

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

✨ Neo Explanation

Routine deployment adding `significance` to the PostHighlight GraphQL API and a per-post `postHighlight` field with TTL-based active-highlight filtering; includes DB and ClickHouse schema migrations. ✅ Low Risk

This is a standard application deployment rolling out commit e23caad (previously 1a072afc). The code changes add a significance field to the PostHighlight GraphQL type, expose a new postHighlight field on the Post type with TTL-based filtering logic, and introduce a postHighlightTtlSeconds remote config variable (defaulting to 12 hours).

The deployment follows the normal pattern: all Deployments and CronJobs get the new container image, the previous migration Jobs are retired, and new DB and ClickHouse migration Jobs run against the new commit. The DB migration job indicates a schema change is being applied — most likely adding the significance column to the post_highlight table.

🔵 Info — The postHighlightTtlSeconds TTL used in the GraphQL query is read from remote config at startup time (getPostHighlightTtlSeconds() is called when the GraphORM object is constructed), not per-request. Changing this value in remote config will require a pod restart to take effect.

Resource Changes

    Name                                                       Type                           Operation
~   vpc-native-expire-super-agent-trial-cron                   kubernetes:batch/v1:CronJob    update
~   vpc-native-check-analytics-report-cron                     kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-clickhouse-cron                  kubernetes:batch/v1:CronJob    update
+   vpc-native-api-clickhouse-migration-0a2dc6e1               kubernetes:batch/v1:Job        create
+   vpc-native-api-db-migration-0a2dc6e1                       kubernetes:batch/v1:Job        create
~   vpc-native-materialize-monthly-best-post-archives-cron     kubernetes:batch/v1:CronJob    update
~   vpc-native-update-tag-materialized-views-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-hourly-notification-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-generate-search-invites-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-user-posts-analytics-refresh-cron               kubernetes:batch/v1:CronJob    update
~   vpc-native-channel-highlights-cron                         kubernetes:batch/v1:CronJob    update
~   vpc-native-user-profile-updated-sync-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-validate-active-users-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-update-trending-cron                            kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-gifted-plus-cron                          kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-old-notifications-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-materialize-yearly-best-post-archives-cron      kubernetes:batch/v1:CronJob    update
~   vpc-native-update-source-public-threshold-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-history-day-clickhouse-cron      kubernetes:batch/v1:CronJob    update
~   vpc-native-update-highlighted-views-cron                   kubernetes:batch/v1:CronJob    update
~   vpc-native-daily-digest-cron                               kubernetes:batch/v1:CronJob    update
~   vpc-native-user-profile-analytics-clickhouse-cron          kubernetes:batch/v1:CronJob    update
~   vpc-native-calculate-top-readers-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-channel-highlights-cron                   kubernetes:batch/v1:CronJob    update
~   vpc-native-bg-deployment                                   kubernetes:apps/v1:Deployment  update
~   vpc-native-clean-zombie-opportunities-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-personalized-digest-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-update-achievement-rarity-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-user-companies-cron                kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-stale-user-transactions-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-channel-digests-cron                            kubernetes:batch/v1:CronJob    update
-   vpc-native-api-clickhouse-migration-1a072afc               kubernetes:batch/v1:Job        delete
~   vpc-native-temporal-deployment                             kubernetes:apps/v1:Deployment  update
~   vpc-native-private-deployment                              kubernetes:apps/v1:Deployment  update
~   vpc-native-update-views-cron                               kubernetes:batch/v1:CronJob    update
~   vpc-native-squad-posts-analytics-refresh-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-users-cron                         kubernetes:batch/v1:CronJob    update
~   vpc-native-personalized-digest-deployment                  kubernetes:apps/v1:Deployment  update
~   vpc-native-deployment                                      kubernetes:apps/v1:Deployment  update
~   vpc-native-user-profile-analytics-history-clickhouse-cron  kubernetes:batch/v1:CronJob    update
~   vpc-native-rotate-weekly-quests-cron                       kubernetes:batch/v1:CronJob    update
... and 12 other changes

@capJavert
Copy link
Copy Markdown
Contributor Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 25, 2026

Claude finished @capJavert's task in 2m 46s —— View job


Claude Code Review

  • Read PR diff and changed files
  • Analyze code changes for bugs, security, performance
  • Check tests coverage
  • Post review feedback

Nice, focused PR — exposes Post.postHighlight via GraphORM with TTL/retired/significance filtering, plus the new significance field on PostHighlight. Overall the shape looks good. A few notes below; the first one is worth a look before merging.

Findings

1. Missing .limit(1) on the postHighlight relation — risk of duplicate parent rows (likely bug)

src/graphorm/index.ts:743-760 declares isMany: false plus an orderBy/addOrderBy, but there's no .limit(1). The DB only enforces uniqueness on (channel, postId) (UQ_post_highlight_channel_post in src/entity/PostHighlight.ts:62), so a single post can legitimately have multiple active highlights across channels (vibes, agentic, …). With multiple matches, the LEFT JOIN can multiply parent rows or yield non-deterministic results depending on how GraphORM materializes the relation.

Compare with the existing precedent for isMany: false + orderBy elsewhere in the same file, which always pairs ordering with .limit(1):

.where(`${childAlias}."userId" = ${parentAlias}.id`)
.orderBy(`${childAlias}."issuedAt"`, 'DESC')
.limit(1),

Suggested change:

postHighlight: {
  relation: {
    isMany: false,
    customRelation: (_, parentAlias, childAlias, qb): QueryBuilder =>
      qb
        .where(`${childAlias}."postId" = ${parentAlias}."id"`)
        .andWhere(`${childAlias}."significance" != :unspecified`, {
          unspecified: PostHighlightSignificance.Unspecified,
        })
        .andWhere(`${childAlias}."retiredAt" IS NULL`)
        .andWhere(
          `${childAlias}."highlightedAt" > now() - (:ttlSeconds || ' seconds')::interval`,
          { ttlSeconds: getPostHighlightTtlSeconds() },
        )
        .orderBy(`${childAlias}."significance"`, 'ASC')
        .addOrderBy(`${childAlias}."highlightedAt"`, 'DESC')
        .limit(1),
  },
},

A regression test that inserts two highlights for the same postId across different channels (e.g. vibes + agentic) and asserts the response is exactly one row, with the more significant tier winning, would lock this in. Fix this →

2. Test coverage gaps for the filter conditions

The new describe('postHighlight field', ...) in __tests__/posts.ts:948 only covers "no highlight" and "one active highlight." The customRelation also filters on retiredAt IS NULL, significance != Unspecified, and the TTL. Each of those is a separate branch worth a one-line case to prevent silent regressions later, e.g.:

  • highlight with significance: PostHighlightSignificance.Unspecifiednull
  • highlight with retiredAt set → null
  • highlight with highlightedAt older than postHighlightTtlSecondsnull

These are short to add and pair naturally with the existing two tests.

3. significance typed as String rather than an enum

src/schema/highlights.ts:60 exposes significance: String. The values are a closed set (breaking | major | notable | routine, or null). A GraphQL enum would self-document the contract for clients and prevent typos from going unnoticed. Not a blocker — fine to keep as String if you want flexibility — but worth a thought.

4. Nit: getPostHighlightTtlSeconds could live in a shared module

src/graphorm/index.ts:101-108 defines getPostHighlightTtlSeconds inline. If/when other code paths (cron cleanup, schema resolvers in src/schema/highlights.ts) need the same TTL, it's a small refactor to colocate the constant + helper near the PostHighlight entity or in src/common/highlights.ts. Optional.

Looks good

  • Filter semantics (significance != Unspecified, retiredAt IS NULL, TTL window) match the intent of the new field.
  • Ordering by significance ASC (lower enum int = more significant) is consistent with majorHeadlineSignificances in src/schema/highlights.ts:97.
  • remoteConfig.vars.postHighlightTtlSeconds is read at call-time, so a runtime change to the remote config takes effect without restart — good (also flagged by the Pulumi/Neo summary).
  • PostHighlight GraphORM mapping (src/graphorm/index.ts:2774-2784) correctly transforms the smallint significance via toPostHighlightSignificanceLabel, returning null for Unspecified, which lines up with the nullable schema type.
  • Tests assert via toEqual({...}) rather than scattered toBes — matches the codebase preference.

· Branch: expose-post-highlight

@capJavert capJavert marked this pull request as ready for review May 25, 2026 12:59
@capJavert capJavert enabled auto-merge (squash) May 25, 2026 12:59
@capJavert capJavert merged commit 6184d6a into main May 25, 2026
9 checks passed
@capJavert capJavert deleted the expose-post-highlight branch May 25, 2026 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant