Skip to content

LCORE-1323: Add text/event-stream Header for Streaming Query#1145

Merged
tisnik merged 2 commits intolightspeed-core:mainfrom
Jdubrick:add-stream-header
Feb 12, 2026
Merged

LCORE-1323: Add text/event-stream Header for Streaming Query#1145
tisnik merged 2 commits intolightspeed-core:mainfrom
Jdubrick:add-stream-header

Conversation

@Jdubrick
Copy link
Contributor

@Jdubrick Jdubrick commented Feb 12, 2026

Description

  • Adds text/event-stream header back after it was dropped during the refactor and defaulted to text/plain

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: Claude
  • Generated by: (e.g., tool name and version; N/A if not used)

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • Bug Fixes

    • Ensure streaming responses default to Server-Sent Events (SSE) for non-text requests while preserving text format when explicitly requested.
  • New Features

    • Introduced a named media-type constant for SSE to standardize streaming content type across endpoints.
  • Tests

    • Added tests validating streaming media-type headers and behavior for text vs. SSE responses.

Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 12, 2026

Walkthrough

Sets a new constant MEDIA_TYPE_EVENT_STREAM and updates streaming endpoints to use a computed response_media_type: use MEDIA_TYPE_TEXT only when explicitly requested, otherwise use MEDIA_TYPE_EVENT_STREAM. Updates OpenAPI response media type and adds a unit test asserting text media-type behavior.

Changes

Cohort / File(s) Summary
Streaming endpoints
src/app/endpoints/streaming_query.py, src/app/endpoints/a2a.py
Introduce and use MEDIA_TYPE_EVENT_STREAM constant; streaming handlers compute response_media_type and set StreamingResponse media type to SSE by default unless request explicitly asks for text.
Constants & models
src/constants.py, src/models/responses.py
Add MEDIA_TYPE_EVENT_STREAM = "text/event-stream" and update StreamingQueryResponse.openapi_response to reference the new constant instead of hard-coded string.
Tests
tests/unit/app/endpoints/test_streaming_query.py
Add test_streaming_query_text_media_type_header and assertions verifying StreamingResponse media type/header for text requests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • tisnik
  • jrobertboos
  • are-ces
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main
Title check ✅ Passed The title accurately describes the main change—extracting the text/event-stream media type into a constant and applying it across streaming endpoints to restore the correct header for streaming queries.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jrobertboos jrobertboos left a comment

Choose a reason for hiding this comment

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

Overall the PR looks good. However could you make an issue in the LCORE JIRA and attach it to the PR, we are just trying to get better about attaching PR's to issues.

The ticket should be in the title like:

LCORE-XXX: Add text/event-stream Header for Streaming Query

As well as in the Closes Ticket section of the PR description.

Thanks so much!

@tisnik tisnik changed the title Add text/event-stream Header for Streaming Query LCORE-1323: Add text/event-stream Header for Streaming Query Feb 12, 2026
@tisnik tisnik merged commit 51d3a85 into lightspeed-core:main Feb 12, 2026
22 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.

3 participants