feat: add feedback API#1259
Conversation
|
👋 Thanks for sending this our way! Before a maintainer reviews the code, we ask community contributors to align with us on the approach first — it keeps your time pointed at changes we can land. The easiest way is to open or find a GitHub issue and discuss the approach with a maintainer there, then link that issue from this PR. If the issue is already assigned to someone else, please check in with them (or with us) before continuing — otherwise two people may end up working on the same task. See our contributing guidelines for the full picture. |
46b9076 to
a7f744b
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
a7f744b to
74e25e6
Compare
74e25e6 to
ccdb79f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ccdb79f. Configure here.
Co-authored-by: Zach Mitchell <zmitchell@fastmail.com>
ccdb79f to
651735a
Compare
|
Tested to be working. 
|
|
@szokeasaurusrex Since you had reviewed the previous PR would you be the correct person to ping? |
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR!
I admittedly have only had time to skim this briefly so far; to do a proper review, I would first need to familiarize myself a bit more with the spec, which would take some time.
In the meantime, I have given some suggestions for simplifying and improving the code. Would appreciate if you can address these 🙏
| /// A User Feedback item. | ||
| /// | ||
| /// Feedback is transmitted as an [`Event`] carrying a `feedback` context, so this variant | ||
| /// wraps an `Event` rather than a [`Feedback`]. Construct it via `EnvelopeItem::from(feedback)` | ||
| /// rather than the raw variant, and use [`EnvelopeItem::as_feedback`] to recover the feedback. | ||
| Feedback(Event<'static>), |
There was a problem hiding this comment.
Given that Feedback events are meant to have a "feedback" context, I would be in favor of introducing a special FeedbackEvent type which forces the "feedback" context to be included.
This would eliminate the code you have which checks for the presence of a "feedback" context at runtime
| pub use self::client_report::Report as ClientReport; | ||
| pub use super::attachment::*; | ||
| pub use super::envelope::*; | ||
| pub use super::feedback::*; |
There was a problem hiding this comment.
I know we have a lot of existing * imports throughout the codebase, but this is really something I would like us to avoid going forward, as it makes it too easy to accidentally introduce unintended public API changes.
Please therefore replace this with an explicit export of only the types which need to be re-exported.
|
Please also check and address the CI failures before the next review round |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1259 +/- ##
==========================================
+ Coverage 73.81% 73.94% +0.13%
==========================================
Files 64 77 +13
Lines 7538 9454 +1916
==========================================
+ Hits 5564 6991 +1427
- Misses 1974 2463 +489 |
2 participants
Continues the work from #1033.
I personally need this feature so though I might as well finish the partial PR. Largely done together with Claude.
Splitted the change of theBased on my investigations, the wholetypefield from aStringto en enum into its own commit.typefield is not actually needed.Issues