Skip to content

Conversation

@ahmed-n-abdeltwab
Copy link
Contributor

@ahmed-n-abdeltwab ahmed-n-abdeltwab commented May 27, 2025

Proposed changes

Add the tmid attribute to the ChatPostMessage channel request body schema to resolve the Python SDK issue when posting messages using tmid.

Issue(s)

Relates to #35923;

Looking forward to your feedback! 🚀

Summary by CodeRabbit

Release Notes

  • Tests

    • Added end-to-end tests for threaded message reply functionality
  • New Features

    • Message posting API now supports threaded replies, enabling messages to link to parent messages through an optional parameter

✏️ Tip: You can customize this high-level summary in your review settings.

@ahmed-n-abdeltwab ahmed-n-abdeltwab requested review from a team as code owners May 27, 2025 16:55
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented May 27, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented May 27, 2025

⚠️ No Changeset found

Latest commit: 1848c2f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ahmed-n-abdeltwab ahmed-n-abdeltwab changed the title fix: Add tmid field to ChatPostMessage schema fix: Add tmid field to ChatPostMessage channel request body schema May 27, 2025
@ahmed-n-abdeltwab ahmed-n-abdeltwab changed the title fix: Add tmid field to ChatPostMessage channel request body schema fix: Add tmid field in the ChatPostMessage channel request body schema May 27, 2025
@ahmed-n-abdeltwab ahmed-n-abdeltwab marked this pull request as draft May 27, 2025 17:44
@codecov
Copy link

codecov bot commented May 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.85%. Comparing base (9e92b49) to head (1848c2f).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #36093      +/-   ##
===========================================
- Coverage    70.86%   70.85%   -0.02%     
===========================================
  Files         3160     3160              
  Lines       109775   109775              
  Branches     19709    19707       -2     
===========================================
- Hits         77795    77777      -18     
- Misses       29957    29969      +12     
- Partials      2023     2029       +6     
Flag Coverage Δ
e2e 60.34% <ø> (+0.01%) ⬆️
e2e-api 47.85% <ø> (-1.06%) ⬇️
unit 72.06% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@CLAassistant
Copy link

CLAassistant commented May 28, 2025

CLA assistant check
All committers have signed the CLA.

@ahmed-n-abdeltwab ahmed-n-abdeltwab force-pushed the fix/python-sdk-chat-postMessage branch from 8811b37 to 3797d55 Compare May 28, 2025 16:33
@ahmed-n-abdeltwab
Copy link
Contributor Author

The tmid field is described in the documentation as The ID of the thread to which the message belongs. I think it's more logical for the roomID to be require when using tmid

@ahmed-n-abdeltwab ahmed-n-abdeltwab marked this pull request as ready for review May 28, 2025 18:32
@sampaiodiego sampaiodiego self-assigned this Jun 5, 2025
@ggazzo ggazzo changed the title fix: Add tmid field in the ChatPostMessage channel request body schema chore: Add tmid field in the ChatPostMessage request body schema Jun 9, 2025
@ggazzo ggazzo added this to the 7.8.0 milestone Jun 9, 2025
@scuciatto scuciatto modified the milestones: 7.9.0, 7.10.0 Jul 22, 2025
@scuciatto scuciatto modified the milestones: 7.10.0, 7.11.0 Aug 25, 2025
@scuciatto scuciatto removed this from the 7.11.0 milestone Sep 18, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

Walkthrough

This pull request extends the Chat API to support threaded replies via the tmid (thread message ID) parameter and adds end-to-end tests validating the threaded reply functionality. The type definitions have been updated to accept tmid in the postMessage payload, and two comprehensive test cases verify the feature works correctly across different room creation methods.

Changes

Cohort / File(s) Summary
Type Definitions
packages/rest-typings/src/v1/chat.ts
Extended ChatPostMessage union type with optional tmid?: string field and corresponding schema update for the roomId-based payload variant.
End-to-End Tests
apps/meteor/tests/end-to-end/api/chat.ts
Added two threaded reply tests: one using channel creation and another using room creation. Both verify that posting a message with tmid correctly links it to the parent message and that retrieval confirms the thread linkage.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Threaded replies now bloom,
Tests hop through each room,
tmid links messages true,
Conversations branch anew! 🌿

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 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 the tmid field to the ChatPostMessage request body schema, which is reflected in both the typings and test files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

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.

7 participants