Conversation
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/sourcehub/bulletin/tx.pulsar.go (1)
4861-4917:⚠️ Potential issue | 🔴 Critical
artifactis accepted but not persisted.The
MsgCreatePostmessage (line 4861) accepts anartifactfield, but the generated code and keeper implementation show a critical mismatch:artifactis emitted only inEventPostCreatedand never stored in thePostmodel. ThePoststruct inx/bulletin/types/post.pb.gohas noartifactfield, andCreatePostinx/bulletin/keeper/msg_server.godrops the artifact when persisting (lines 109–114), only forwarding it to the event (line 123). Clients can submit this field and see it in the event stream, but any later post retrieval loses it. Addartifactto thePostmessage definition, persist it inCreatePost, and regenerate the protobuf outputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/sourcehub/bulletin/tx.pulsar.go` around lines 4861 - 4917, The Post artifact field is accepted in MsgCreatePost and emitted in EventPostCreated but never persisted; update the Post protobuf message (the Post struct in x/bulletin/types/post.proto/.pb.go) to include an artifact string field, modify the CreatePost handler in x/bulletin/keeper/msg_server.go to copy msg.Artifact into the stored Post before saving, and then regenerate protobuf code so MsgCreatePost, Post, and related .pb.go files are consistent and the artifact is persisted and returned on post retrieval.
🧹 Nitpick comments (1)
api/sourcehub/bulletin/events.pulsar.go (1)
2370-2427: Add one round-trip regression test forArtifact.Given the PR note about the unavailable generator toolchain, a small marshal/unmarshal assertion for
EventPostCreated{Artifact: "..."}would be a cheap guard against future drift between the struct, reflection path, and raw descriptor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/sourcehub/bulletin/events.pulsar.go` around lines 2370 - 2427, Add a regression test that ensures EventPostCreated.Artifact survives protobuf marshal/unmarshal round-trips: create an instance of EventPostCreated with Artifact set (e.g., "test-artifact"), marshal it with proto.Marshal (using the generated message type EventPostCreated), then unmarshal into a new EventPostCreated and assert the Artifact field equals the original; use the generated methods (proto.Marshal/proto.Unmarshal and EventPostCreated) and include this test alongside other message tests so future changes to the struct, Descriptor, or reflection path are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/sourcehub/bulletin/tx.pulsar.go`:
- Around line 4861-4917: The Post artifact field is accepted in MsgCreatePost
and emitted in EventPostCreated but never persisted; update the Post protobuf
message (the Post struct in x/bulletin/types/post.proto/.pb.go) to include an
artifact string field, modify the CreatePost handler in
x/bulletin/keeper/msg_server.go to copy msg.Artifact into the stored Post before
saving, and then regenerate protobuf code so MsgCreatePost, Post, and related
.pb.go files are consistent and the artifact is persisted and returned on post
retrieval.
---
Nitpick comments:
In `@api/sourcehub/bulletin/events.pulsar.go`:
- Around line 2370-2427: Add a regression test that ensures
EventPostCreated.Artifact survives protobuf marshal/unmarshal round-trips:
create an instance of EventPostCreated with Artifact set (e.g.,
"test-artifact"), marshal it with proto.Marshal (using the generated message
type EventPostCreated), then unmarshal into a new EventPostCreated and assert
the Artifact field equals the original; use the generated methods
(proto.Marshal/proto.Unmarshal and EventPostCreated) and include this test
alongside other message tests so future changes to the struct, Descriptor, or
reflection path are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d43dd738-97f2-4f18-b76b-4358c610b27e
⛔ Files ignored due to path filters (1)
api/sourcehub/bulletin/query_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
api/sourcehub/bulletin/events.pulsar.goapi/sourcehub/bulletin/query.pulsar.goapi/sourcehub/bulletin/tx.pulsar.go
📜 Review details
⏰ 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: test
🔇 Additional comments (4)
api/sourcehub/bulletin/events.pulsar.go (1)
586-1220: LGTM:artifactis plumbed consistently across the generated message.Descriptor lookup, reflection helpers, size/marshal/unmarshal, the exported field, getter, and raw descriptor bytes all agree on a length-delimited string at field
5. That also matches the typed event shape inx/bulletin/types/events.pb.go:95-106.Also applies to: 2370-2427, 2549-2587
api/sourcehub/bulletin/query.pulsar.go (3)
6744-7518:QueryBulletinPolicyId*matches the proto contract.The empty request and single
policy_idresponse are wired consistently through the raw descriptor, fast-reflection helpers, concrete structs, and getter.Also applies to: 9209-9270, 9487-9493
7543-7545: The message-slot renumbering is internally consistent.The promotion of
QueryBulletinPolicyId*to slots14/15and the resulting move ofQueryIterateGlob*to16/17is reflected coherently in the descriptor bytes, reset/descriptor paths, type metadata, dependency indexes, exporter slots, and message count.Also applies to: 8155-8157, 9284-9301, 9335-9352, 9494-9511, 9631-9684, 9880-9935
9512-9616: No action needed—the generated RPC stubs are complete.
BulletinPolicyIdis already fully exposed inquery_grpc.pb.go, including the client method, server interface, unimplemented base, handler, and service registration. The descriptor inquery.pulsar.goaligns with the callable transport layer.> Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #103 +/- ##
==========================================
+ Coverage 47.80% 47.86% +0.05%
==========================================
Files 276 276
Lines 16196 16208 +12
==========================================
+ Hits 7743 7758 +15
+ Misses 7647 7645 -2
+ Partials 806 805 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
These were not generated due to me not having protoc-gen-go-grpc