Skip to content

feat: Add origin header to ThreadContext::HeaderIdBundle for internal…#6297

Merged
tewaro merged 1 commit intomainfrom
tewaro/add-origin-header
Mar 11, 2026
Merged

feat: Add origin header to ThreadContext::HeaderIdBundle for internal…#6297
tewaro merged 1 commit intomainfrom
tewaro/add-origin-header

Conversation

@tewaro
Copy link
Contributor

@tewaro tewaro commented Mar 11, 2026

… use

@tewaro tewaro requested review from a team as code owners March 11, 2026 14:41
Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

I'm Bonk, and I've done a quick review of your PR.

This PR adds an origin (Origin) header ID to ThreadContext::HeaderIdBundle.

Issues (ranked by severity):

  1. [MEDIUM] No usages of the new field. The origin header ID is registered in the bundle but is not referenced anywhere in the codebase. Every other field in HeaderIdBundle has a comment documenting its purpose (e.g., // used by cache API implementation, // used in tracing instrumentation). Without a consumer, this is dead code — it increases the header table size for every thread context with no benefit. Could you either (a) add the code that uses it in this PR, or (b) explain the intended consumer so reviewers can evaluate whether this addition is appropriate?

  2. [LOW] Missing usage comment. All other fields in the struct have a // used by … comment. The new field should follow the same convention.

See inline comments for details.


This review was generated by an AI assistant and may contain errors. Please verify all suggestions before applying.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 11, 2026

I'm Bonk, and I've done a quick review of your PR.

This PR adds an Origin header ID to ThreadContext::HeaderIdBundle. I posted a review with two findings:

  1. [MEDIUM] No usages of the new field. The origin header ID is registered but never referenced anywhere in the codebase. Without a consumer, this is dead code that increases the header table size for every thread context. The PR should either include the code that uses this field or explain the intended use case.

  2. [LOW] Missing usage comment. Every other field in HeaderIdBundle has a // used by … comment documenting its purpose. The new field lacks one.

Review posted at: #6297 (review)

github run

@tewaro tewaro enabled auto-merge March 11, 2026 15:06
@tewaro tewaro disabled auto-merge March 11, 2026 16:40
@tewaro tewaro merged commit 8c41032 into main Mar 11, 2026
28 of 30 checks passed
@tewaro tewaro deleted the tewaro/add-origin-header branch March 11, 2026 16:46
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