fix: [#4902][onTurnError] unhandled error for informative streaming#571
fix: [#4902][onTurnError] unhandled error for informative streaming#571ceciliaavila wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds top-level additionalProperties to the Entity CompositeMapper and removes the additionalProperties block from the inline Entity within Activity.entities items in mappers.ts. No other files changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libraries/botframework-connector/src/connectorApi/models/mappers.ts (2)
402-407: LGTM on behavior; tidy Prettier nits on additionalProperties.Adding
additionalPropertiesat the Entity level is the right fix for core-client. Prettier flags quotes/trailing commas here—patch below.- additionalProperties: { - type: { - name: "Object" - } - } + additionalProperties: { + type: { + name: 'Object', + }, + }
717-721: Inline Entity mapper change is correct; fix style to satisfy Prettier.Pointing the sequence element to
Entity(and removing the per-itemadditionalProperties) is correct now thatEntitycarries it. Prettier wants single quotes and a trailing comma.- className: "Entity" + className: 'Entity',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries/botframework-connector/src/connectorApi/models/mappers.ts(2 hunks)
🧰 Additional context used
🪛 ESLint
libraries/botframework-connector/src/connectorApi/models/mappers.ts
[error] 402-402: Replace ···· with ········
(prettier/prettier)
[error] 403-403: Replace ···· with ········
(prettier/prettier)
[error] 404-404: Insert ······
(prettier/prettier)
[error] 405-405: Replace ········name:·"Object" with ················name:·'Object',
(prettier/prettier)
[error] 406-406: Replace ······} with ············},
(prettier/prettier)
[error] 719-719: Replace ··············className:·"Entity" with ····························className:·'Entity',
(prettier/prettier)
⏰ 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). (11)
- GitHub Check: lint
- GitHub Check: test:schemas
- GitHub Check: tests.yml ubuntu (node 18.x)
- GitHub Check: depcheck
- GitHub Check: tests.yml windows (node 20.x)
- GitHub Check: tests.yml windows (node 18.x)
- GitHub Check: tests.yml ubuntu (node 22.x)
- GitHub Check: tests.yml windows (node 22.x)
- GitHub Check: tests.yml ubuntu (node 20.x)
- GitHub Check: test:compat
- GitHub Check: test:consumer
|
/promoted 4903 |
Fixes # 4902
#minor
Description
After migrating from azure/core-http to azure/core-client package, the CompositeMapper changed its behavior regarding additional properties. Now, they must be defined in the model.
This PR fixes the issue of unmapped additional properties in the Entity by explicitly incorporating the object into the Entity model.
Specific Changes
connectorApi/models/mappersincluding theadditionalPropertiesobject in the Entity model. Additionally, we removed it from the entities array as it was redundant.Testing
This image shows the informative streamInfo message being displayed after applying the fix.
