Skip to content

feat(HNT-2887): add crawl domain message and event types - #19

Merged
Herraj merged 8 commits into
mainfrom
claude/hnt-2086-domain-types
Sep 1, 2026
Merged

feat(HNT-2887): add crawl domain message and event types#19
Herraj merged 8 commits into
mainfrom
claude/hnt-2086-domain-types

Conversation

@mmiermans

@mmiermans mmiermans commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Goal

HNT-2887

Add the discovery-side and agent-side crawl domain types so the validators, handlers, and consumers that follow reason against one shared set of interfaces. This slice introduces DiscoveryContext, CrawlArticleDiscoveryMessage, LiveArticle, and PublisherList in the messages module, ArticleDiscoveryEvent in the events module, and an optional refresh_interval_minutes on CrawlArticleMessage, all re-exported from the crawl-common barrel. The shapes to reason about are the discovery job, the discovery event that becomes a crawl.article_discoveries row, and the publisher list the agent loads from JSON.

See docs/ARCHITECTURE.md in #14 for the message and event contracts these types encode.

Implementation decisions

Decision Approach Why
Make refresh_interval_minutes optional on CrawlArticleMessage Declare it as an optional field, and the worker falls back to its configured fetch TTL when it is absent A rolling deploy must not reject messages that were enqueued before the field existed, which is the additive-only schema evolution that Google Cloud Pub/Sub schema evolution is designed to support: add the field, keep it optional, then depend on it.

@mmiermans
mmiermans force-pushed the claude/hnt-2086-domain-types branch from c389458 to 5da2693 Compare July 6, 2026 17:08
@mmiermans mmiermans changed the title feat(HNT-2086): add crawl domain message and event types feat(HNT-2887): add crawl domain message and event types Aug 6, 2026
@mmiermans

Copy link
Copy Markdown
Collaborator Author

Rename the refresh-window message fields

Rename CrawlArticleDiscoveryMessage.interval_minutes to page_refresh_minutes and CrawlArticleMessage.refresh_interval_minutes to article_refresh_minutes, both in packages/crawl-common/src/types/messages.ts, so message fields and env vars form one family in which "refresh" always means how often content is re-extracted. Nothing is in production, so there is no wire-compatibility cost.

Downstream callers follow the rename rather than living here: the message validators in #20, the publisher-list fixtures in #36, processDiscovery in #30, and processArticle plus the tick's job construction in #29 and #37.

Comment thread packages/crawl-common/src/types/events.ts
@mmiermans

Copy link
Copy Markdown
Collaborator Author

Drop interval_minutes from CrawlArticleDiscoveryMessage

The page crawl cadence becomes a single global configuration value, read by the scheduler when it decides which pages are due (#35, #37) and by the discovery worker for its page freshness check (#25, #30). The message field is therefore removed rather than kept, here in packages/crawl-common/src/types/messages.ts. Note that this reaches the publisher list too, since PublisherList.pages is typed as CrawlArticleDiscoveryMessage[]: dropping the field from the interface is what removes it from every committed page entry (#38), and the boundary validation for it goes at the same time (#20).

This supersedes the earlier request on this PR to rename the field to page_refresh_minutes. It is not being renamed, it is going away. The other half of that comment still stands: CrawlArticleMessage.refresh_interval_minutes is a different field on a different message, it is still producer-set per job, and renaming it to article_refresh_minutes is unaffected by this decision.

The reason for removing it is that the per-page capability has never been used. All 3,399 pages carry interval_minutes: 20, without a single differing value, and carrying it per page makes the one value we actually change expensive to change: a cadence adjustment currently means re-running the generator and reviewing a diff that touches every entry. The architecture doc calls the page crawl interval the main lever on Zyte volume, so it is at once the setting most likely to be tuned and the hardest to tune.

The trade-off being accepted, worth stating plainly since it is a property of the message shape and so belongs on this PR: once jobs stop carrying their own window, a job already on the queue is evaluated against whatever the consuming worker's setting says at handling time rather than the value in force when it was published. That is fine for a value that is uniform and changes rarely, but it is genuinely being given up rather than preserved.

The capability is deferred, not abandoned. If we later want per-page or per-topic differentiation, and the FPS "Zyte Cost Optimization" page does propose slower crawling for evergreen topics and overnight, the field can be reintroduced deliberately at that point, with the same "nothing in production" freedom we have now.

@Herraj
Herraj marked this pull request as ready for review August 27, 2026 17:26
headline?: string;
authors?: ArticleAuthor[];
summary?: string;
language?: string;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good choice to keep this as a string; Zyte can potentially reply with any of the ISO-standard language codes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment here with the ISO format that this field is in based on the Zyte docs

// worker gates re-fetch and writes its fetch claim against this value.
// Optional so a rolling deploy does not reject messages enqueued before
// the field existed; the worker falls back to the configured fetch TTL.
refresh_interval_minutes?: number;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional so a rolling deploy does not reject messages enqueued before the field existed

The crawling pipeline is greenfield work and not in production, so we shouldn't worry about breaking existing messages. Perhaps this comment is attributing the reason for the optionality incorrectly? Is it really there because we have a default refresh interval that's not sent along the message?

Would it be better just to send the refresh interval anyway, so this doesn't have to be optional?

/**
* Surface and topic a discovered page is crawled for. A page
* can be crawled for several surfaces, so each discovery job
* carries one context per (surface, topic) pair.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an example of a surface (NEW_TAB_EN_US) and topic (BUSINESS?) to make it easier to read for new devs.

@mmiermans mmiermans left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ I literally cannot approve pull requests. 😄 This may be something we'll have to address.. In any case, I approve. You can approve as well and merge.

@mmiermans
mmiermans changed the base branch from claude/hnt-2086-redis-state-package to main August 27, 2026 23:48
@Herraj
Herraj merged commit 059e49d into main Sep 1, 2026
3 checks passed
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.

2 participants