Skip to content

[GitHub-216] Completion detail view was streamed#460

Open
jacekzimonski wants to merge 6 commits intomainfrom
jacek/GitHub-216-Completion-detail-view-was-streamed
Open

[GitHub-216] Completion detail view was streamed#460
jacekzimonski wants to merge 6 commits intomainfrom
jacek/GitHub-216-Completion-detail-view-was-streamed

Conversation

@jacekzimonski
Copy link
Collaborator

Closes:

#216

Screenshots:

Screenshot 2025-10-10 at 3 00 06 PM Screenshot 2025-10-10 at 3 01 14 PM

@vercel
Copy link

vercel bot commented Oct 10, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
anotherai Ready Ready Preview Comment Oct 13, 2025 8:57am
anotherai-docs Ready Ready Preview Comment Oct 13, 2025 8:57am

Copy link
Member

@guillaq guillaq left a comment

Choose a reason for hiding this comment

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

Just add a metadata when stream=True

@jacekzimonski
Copy link
Collaborator Author

Making the change right now

@claude
Copy link
Contributor

claude bot commented Oct 10, 2025

Claude finished @jacekzimonski's task —— View job


PR Review: [GitHub-216] Completion detail view was streamed

This PR implements the ability to track streaming status in completion metadata. The changes are well-implemented with good attention to existing patterns. Here's my detailed review:

Code Quality & Best Practices

Backend Changes:

  • completion_runner.py: Clean implementation that only adds stream: true metadata when streaming is enabled, following the existing metadata pattern
  • ClickHouse model: Simple schema addition without breaking changes
  • Services: Both playground and run services properly pass stream parameter to completion runner

Frontend Changes:

  • Frontend cleanup: Appropriate removal of unused stream-related code from components
  • Types: Clean removal of stream property from models

Security & Performance

  • No security concerns: Changes only add metadata tracking, no sensitive information exposed
  • Minimal performance impact: Only adds a metadata entry when stream=True, negligible overhead
  • No SQL injection risks: Uses structured metadata dictionary, not raw queries

🔍 Detailed Review by File

Backend Files

backend/core/services/completion_runner.py:114-119

# Add stream information to metadata (only when True)
metadata_with_stream = {**metadata}
if stream:
    metadata_with_stream["stream"] = True

Excellent approach - Only adds metadata when stream=True, avoiding noise for non-streaming completions.

backend/core/storage/clickhouse/_models/_ch_completion.py:127

metadata: dict[str, str] = Field(default_factory=dict)

No changes needed - Existing metadata field handles the new stream property correctly.

backend/protocol/api/_services/playground_service.py:194

# Playground completions are never streamed, so no need to add stream metadata

Good documentation - Clear comment explains why no stream metadata is added.

backend/protocol/api/_services/run/run_service.py:130-133

# Add stream metadata for cached completions (only when True)
metadata_with_stream = {**prepared_run.metadata}
if stream:
    metadata_with_stream["stream"] = True

Consistent implementation - Same pattern as completion_runner.py for consistency.

Frontend Files

All frontend changes involve removing unused stream-related code:

  • CompletionTableVersionCell.tsx: Removed unused stream handling
  • utils.ts: Removed stream from ignored keys and display mappings
  • VersionDetailsView.tsx: Removed stream from displayed keys
  • models.ts: Removed stream property from Version interface

Clean removal - These removals make sense as streaming is now tracked in metadata rather than version properties.

🔍 Potential Issues

Minor considerations:

  1. Backward compatibility: The metadata approach is backward-compatible - existing completions without stream metadata will work fine
  2. Consistency: The stream: true only approach is good, but ensure frontend can distinguish between "not streamed" vs "unknown streaming status" if needed
  3. Testing: No test changes visible - consider adding tests for metadata inclusion

📋 Code Review Summary

Strengths:

  • Clean, minimal implementation
  • Consistent patterns across services
  • Good separation of concerns
  • Backward compatible changes
  • Appropriate use of conditional metadata addition

Areas for improvement:

  • Consider adding unit tests for the new metadata behavior
  • Documentation could mention the metadata approach in API docs

Overall Assessment

This is a well-implemented feature that correctly addresses the issue #216. The code follows existing patterns, maintains backward compatibility, and has minimal performance impact. The approach of using metadata rather than version properties is appropriate for runtime behavior tracking.

Recommendation: ✅ Approve - The implementation is solid and ready for merge.


@jacekzimonski
Copy link
Collaborator Author

@guillaq
Fixes where committed, now by default when it's false we are not adding it to the metadata

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