Skip to content

fix: Ensure the id is part of the message pydantic model - #10553

Closed
erichare wants to merge 5 commits into
mainfrom
fix-message-id-missing
Closed

fix: Ensure the id is part of the message pydantic model#10553
erichare wants to merge 5 commits into
mainfrom
fix-message-id-missing

Conversation

@erichare

@erichare erichare commented Nov 10, 2025

Copy link
Copy Markdown
Member

This pull request introduces a small change to the Message class in src/lfx/src/lfx/schema/message.py by adding an optional id field. This allows each message to have a unique identifier, which can be useful for tracking or referencing messages in the system.

Summary by CodeRabbit

  • New Features
    • Messages now support optional unique identifiers, enabling improved tracking and reference capabilities across the system.

@coderabbitai

coderabbitai Bot commented Nov 10, 2025

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

A new optional id field is added to the Message schema model with type UUID | str | None and default value None, allowing messages to carry optional unique identifiers without altering existing functionality.

Changes

Cohort / File(s) Summary
Message Schema Update
src/lfx/src/lfx/schema/message.py
Added optional id field of type UUID | str | None with default None to Message model

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

  • Verify the field type union (UUID | str | None) aligns with intended usage and serialization behavior
  • Confirm that the default value of None is appropriate for the expected workflow
  • Check whether any validation logic or constraints should be applied to the new field

Pre-merge checks and finishing touches

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 2 inconclusive)
Check name Status Explanation Resolution
Test Coverage For New Implementations ❌ Error PR adds new id field to Message model but existing test file contains no test coverage for the id field. Add test cases for UUID id, string id, None default, and serialization with id field to test_schema_message.py.
Test Quality And Coverage ❓ Inconclusive Cannot locate modified file src/lfx/src/lfx/schema/message.py or associated test files in repository despite extensive search attempts. Provide access to the actual repository containing the changes or direct access to the test files and PR diff to enable assessment.
Test File Naming And Structure ❓ Inconclusive PR modifies message.py but includes no test files to evaluate against test pattern and structure requirements. Add test file(s) with proper naming (test_*.py), pytest structure, descriptive function names, and comprehensive edge case coverage for the new id field.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding an id field to the Message pydantic model. It's specific, concise, and directly reflects the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Excessive Mock Usage Warning ✅ Passed The PR contains only schema changes to the Message model with no test file modifications or additions, therefore excessive mock usage cannot be assessed.

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.

@github-actions github-actions Bot added the bug Something isn't working label Nov 10, 2025
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Nov 10, 2025
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Nov 10, 2025
@github-actions

Copy link
Copy Markdown
Contributor

Frontend Unit Test Coverage Report

Coverage Summary

Lines Statements Branches Functions
Coverage: 15%
14.67% (3955/26947) 7.45% (1533/20552) 9% (532/5906)

Unit Test Results

Tests Skipped Failures Errors Time
1588 0 💤 0 ❌ 0 🔥 19.29s ⏱️

@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Nov 10, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lfx/src/lfx/schema/message.py (1)

380-394: Propagate the id field from Message to MessageResponse.

The from_message method doesn't copy the newly added id field from the source Message to the MessageResponse. Since both models now have this field and the PR's purpose is to enable message tracking/referencing, the identifier should be preserved during conversion.

Apply this diff to propagate the id field:

         return cls(
+            id=message.id,
             sender=message.sender,
             sender_name=message.sender_name,
             text=message.text,
             session_id=message.session_id,
             context_id=message.context_id,
             files=message.files or [],
             timestamp=message.timestamp,
             flow_id=flow_id,
         )
🧹 Nitpick comments (1)
src/lfx/src/lfx/schema/message.py (1)

38-38: Consider adding a field serializer for the id field.

The flow_id field (line 48) with similar typing has both a validator (lines 57-62) and serializer (lines 86-90) to ensure UUID values are consistently converted to strings. Consider adding similar handlers for the id field to maintain consistency in serialization behavior across the codebase.

Apply this diff to add consistent UUID handling:

     @field_validator("flow_id", mode="before")
     @classmethod
     def validate_flow_id(cls, value):
         if isinstance(value, UUID):
             value = str(value)
         return value
 
+    @field_validator("id", mode="before")
+    @classmethod
+    def validate_id(cls, value):
+        if isinstance(value, UUID):
+            value = str(value)
+        return value
+
     @field_validator("content_blocks", mode="before")

And add a corresponding serializer after line 90:

     @field_serializer("flow_id")
     def serialize_flow_id(self, value):
         if isinstance(value, UUID):
             return str(value)
         return value
 
+    @field_serializer("id")
+    def serialize_id(self, value):
+        if isinstance(value, UUID):
+            return str(value)
+        return value
+
     @field_serializer("timestamp")
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a34d84 and 7b7d424.

📒 Files selected for processing (1)
  • src/lfx/src/lfx/schema/message.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Update Starter Projects
🔇 Additional comments (1)
src/lfx/src/lfx/schema/message.py (1)

38-38: LGTM! The new id field follows established patterns.

The optional id field is well-defined with appropriate typing and default value, consistent with similar fields like flow_id.

@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Nov 10, 2025
@erichare
erichare enabled auto-merge November 10, 2025 21:53
@erichare
erichare disabled auto-merge November 10, 2025 21:53
@erichare erichare closed this Nov 12, 2025
@erichare
erichare deleted the fix-message-id-missing branch November 12, 2025 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant