Support pinned post #86
Conversation
…eedDefsReasonRepost` to support `reasonPin`
WalkthroughThe changes involve enhancements to several classes related to user profiles and feed management in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsViewerState.kt (1)
11-11: LGTM! Consider adding a brief comment for clarity.The addition of the
pinnedproperty aligns well with the PR objectives and follows the existing coding style. It's correctly implemented as a nullable Boolean, which is consistent with other properties in the class.Consider adding a brief comment to explain the purpose of this property, as it represents a new feature:
/** Indicates whether the post is pinned in the user's post list. */ var pinned: Boolean? = nullcore/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonPin.kt (1)
7-16: LGTM: Class structure is well-defined, with a minor suggestion.The
FeedDefsReasonPinclass is correctly annotated for serialization and extendsFeedDefsReasonUnion, which aligns with the PR objectives of modifying post reasoning. The companion object and type override are implemented correctly.Consider adding a KDoc comment to the class to describe its purpose and relationship to pinned posts. This would improve code documentation and maintainability. For example:
/** * Represents a reason for a post being pinned in a feed. * This class is used in serialization/deserialization of feed items * where a post is pinned to the top of a user's profile. */ @Serializable class FeedDefsReasonPin : FeedDefsReasonUnion() { // ... existing code ... }core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonUnion.kt (1)
7-12: LGTM: Well-structured abstract class with appropriate serialization.The
FeedDefsReasonUnionclass is well-defined as an abstract class with a custom polymorphic serializer. This structure is suitable for representing different types of feed reasons.Consider enhancing the KDoc by briefly explaining the purpose of this class and how it relates to
FeedDefsReasonRepostandFeedDefsReasonPin. For example:/** * Abstract base class for representing different types of feed reasons. * Subclasses include: * @see FeedDefsReasonRepost * @see FeedDefsReasonPin */core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/actor/ActorUpdateProfileRequest.kt (1)
15-17: LGTM: New properties for pinned post support added correctly.The new properties
pinnedPostandclearPinnedPostare well-implemented and align with the PR objectives. The nullableRepoStrongRef?type forpinnedPostand the boolean flag forclearPinnedPostprovide the necessary flexibility for managing pinned posts.Consider updating the comment above
clearPinnedPostto be more consistent with the comment style used forclearBanner:- // set true if you want to clear the pinned post + // set true if you want to clear the pinned postThis minor change would improve consistency across the class.
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/actor/ActorProfile.kt (1)
6-6: LGTM! Changes align well with PR objectives.The addition of the
pinnedPostproperty and its corresponding import statement are correctly implemented and align perfectly with the PR objectives to support pinned posts functionality in user profiles.Consider adding a brief comment above the
pinnedPostproperty to explain its purpose and usage. This can help other developers understand the significance of this field in theActorProfileclass. For example:// Reference to the user's pinned post, if any var pinnedPost: RepoStrongRef? = nullAlso applies to: 24-24
core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/PinnedPostSerializer.kt (1)
14-23: LGTM: Effective handling of current and legacy pinned post formats.The
transformDeserializemethod successfully handles both current and legacy formats of pinned posts, ensuring backward compatibility. The fallback mechanism for the old format is well-implemented and clearly commented.Consider adding a brief comment explaining the expected structure of the current format (JsonObject) for improved clarity and maintainability. For example:
override fun transformDeserialize(element: JsonElement): JsonElement { return if (element is JsonObject) { // Current format: {"uri": "...", "cid": "..."} element } else { // fallback for old pinned post format JsonObject(mapOf("uri" to element, "cid" to JsonPrimitive(""))) } }core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/feed/FeedGetAuthorFeedRequest.kt (1)
24-32: Consider updating documentation and related code.The changes to
FeedGetAuthorFeedRequestare well-implemented and align with the PR objective of supporting pinned posts. The addition of theincludePinsproperty with a default value offalseensures backward compatibility.To fully integrate this change:
- Update any relevant documentation to explain the new
includePinsparameter.- Review and update any code that creates or handles
FeedGetAuthorFeedRequestobjects to make use of this new capability where appropriate.- Consider adding unit tests to verify the behavior of requests with
includePinsset to bothtrueandfalse.core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/actor/ActorDefsProfileViewDetailed.kt (1)
26-30: LGTM: Well-implemented new property with appropriate serialization.The
pinnedPostproperty is correctly implemented with the following positive aspects:
- Use of a custom serializer to handle the string type declaration used by some clients.
- Nullable type, allowing for profiles without pinned posts.
- Clear commenting explaining the reason for the custom serializer.
Consider slightly rewording the comment for clarity:
- // In Japan, many clients are using "string type declaration" for pinnedPost field, - // like "pinnedPost": "at://did:plc:xxx". - // Parse the string type declaration as uri of RepoStrongRef by using [PinnedPostSerializer] + // Some clients, particularly in Japan, use a string type declaration for the pinnedPost field, + // e.g., "pinnedPost": "at://did:plc:xxx". + // [PinnedPostSerializer] parses this string declaration as a URI of type RepoStrongRef.core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/FeedDefsReasonPolymorphicSerializer.kt (2)
17-28: LGTM with a suggestion: Consider using a proper logging framework.The
selectDeserializermethod correctly implements the required functionality to select the appropriate serializer based on the type, including support forFeedDefsReasonPinas per the PR objectives.Consider replacing
printlnwith a proper logging framework for better control and flexibility in a production environment. For example:import mu.KotlinLogging private val logger = KotlinLogging.logger {} // In the else block: logger.warn { "Unknown Item type: $type (FeedDefsReasonUnion)" }This change would require adding a logging dependency to your project if not already present.
30-33: LGTM with a suggestion: Consider making theUnknownclass more flexible.The
Unknownclass is correctly implemented as a serializable subclass ofFeedDefsReasonUnionwith a fixed "unknown" type.For increased flexibility, consider allowing the type to be set in the constructor:
@Serializable class Unknown(override var type: String = "unknown") : FeedDefsReasonUnion()This change would allow for different "unknown" types if needed in the future, while maintaining the default "unknown" value.
core/src/commonMain/kotlin/work/socialhub/kbsky/internal/app/bsky/_ActorResource.kt (1)
96-101: New feature implemented: Pinned post handlingThe addition of the conditional block for handling the
pinnedPostattribute is well-implemented and aligns with the PR objectives. It correctly manages both setting and clearing of pinned posts.However, consider adding validation for the
pinnedPostvalue to ensure it meets any required format or constraints before assignment.Consider adding validation for the
pinnedPostvalue:if (request.clearPinnedPost) { it.pinnedPost = null } else { val newPinnedPost = request.pinnedPost ?: originalActorProfile.pinnedPost // Add validation here if (isValidPinnedPost(newPinnedPost)) { it.pinnedPost = newPinnedPost } else { // Handle invalid pinned post (e.g., log warning, throw exception) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/actor/ActorUpdateProfileRequest.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/feed/FeedGetAuthorFeedRequest.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/internal/app/bsky/_ActorResource.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/actor/ActorDefsProfileViewDetailed.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/actor/ActorProfile.kt (2 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsFeedViewPost.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonPin.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonRepost.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonUnion.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsViewerState.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/FeedDefsReasonPolymorphicSerializer.kt (1 hunks)
- core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/PinnedPostSerializer.kt (1 hunks)
🔇 Additional comments (29)
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsFeedViewPost.kt (1)
9-9: Approved change, but additional actions required.The change from
FeedDefsReasonRepost?toFeedDefsReasonUnion?aligns with the PR objective of supporting the newreasonPintype and accommodating increasing instances of pinned posts. This change provides more flexibility for different types of post reasons.However, please consider the following actions:
Add a comment explaining the
FeedDefsReasonUniontype and why this change was made. This will help future developers understand the reasoning behind this modification.Ensure that all existing code that uses the
reasonproperty is updated to handle the new union type correctly.To verify the impact of this change, please run the following script:
This script will help identify areas of the codebase that might need updates due to this change.
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonPin.kt (4)
1-6: LGTM: Package declaration and imports are well-organized.The package declaration follows Kotlin naming conventions, and the imports are correctly organized and necessary for the class implementation. The empty line between the package declaration and imports improves readability.
10-12: LGTM: Companion object is well-implemented.The companion object correctly defines the
TYPEconstant, which is constructed usingBlueskyTypes.FeedDefsand"#reasonPin". This approach ensures consistent type identification for serialization and aligns with the Bluesky ecosystem's conventions.
14-15: LGTM: Type property override is correctly implemented.The
typeproperty is properly overridden and annotated with@SerialName("\$type"), ensuring correct serialization. Initializing it with theTYPEconstant from the companion object maintains consistency and allows for proper type discrimination during deserialization.
1-16: LGTM: Implementation aligns well with PR objectives.The
FeedDefsReasonPinclass is a well-structured implementation that supports the addition of pinned post functionality as described in the PR objectives. It correctly extendsFeedDefsReasonUnionand uses appropriate serialization annotations, allowing for the newreasonPintype inFeedDefsFeedViewPost.reason.This implementation contributes to the enhanced user experience by providing the necessary data structure for handling pinned posts within the platform. It's consistent with the existing codebase structure and serialization patterns, which should facilitate smooth integration with other components.
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonRepost.kt (5)
3-5: LGTM: Import statements are correctly updated.The new import statements for
SerialNameandBlueskyTypesare consistent with the changes made in the class.
11-13: LGTM: Companion object with TYPE constant added.The addition of the companion object with the
TYPEconstant provides a centralized and consistent way to define the type string for this class. This is good practice for maintaining type consistency across the codebase.
15-16: LGTM: Type property correctly overridden and annotated.The
typeproperty is properly overridden and annotated with@SerialName("\$type"). This ensures correct serialization and is consistent with the class's new inheritance structure. Using theTYPEconstant from the companion object promotes consistency.
1-20: Summary: Changes successfully implement part of the "Support pinned post" feature.The modifications to
FeedDefsReasonRepostclass provide a more flexible structure for different reason types in feed posts. The changes are well-implemented and consistent with the PR objectives. Key points:
- The class now inherits from
FeedDefsReasonUnion, allowing for different reason types.- A companion object with a
TYPEconstant has been added for consistency.- The
typeproperty is correctly overridden and annotated for proper serialization.These changes lay the groundwork for supporting pinned posts and other potential reason types in the future.
9-9: LGTM: Class now inherits from FeedDefsReasonUnion.This change aligns with the PR objective of modifying post reasoning to support new types like
reasonPin. It provides a more flexible structure for different reason types.To ensure this change doesn't break existing code, please run the following script:
✅ Verification successful
Verified: Inheritance change is safe.
The inheritance change of
FeedDefsReasonReposttoFeedDefsReasonUniondoes not introduce any direct instantiations or unsafe casts that could affect existing functionality. All references are either imports or safe casts, ensuring stability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any direct instantiations or casts of FeedDefsReasonRepost # that might be affected by the inheritance change. rg --type kotlin -e "FeedDefsReasonRepost\(\)" -e "as FeedDefsReasonRepost"Length of output: 77
Script:
#!/bin/bash # Description: Search for any references to FeedDefsReasonRepost in the codebase to ensure no hidden usages. rg --type kotlin "FeedDefsReasonRepost"Length of output: 794
core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonUnion.kt (3)
1-5: LGTM: Package structure and imports are well-organized.The package declaration and imports are appropriate for the class being defined. The use of a custom polymorphic serializer (
FeedDefsReasonPolymorphicSerializer) is noted and aligns with the polymorphic nature of theFeedDefsReasonUnionclass.
13-14: LGTM: Well-defined type property for polymorphic serialization.The abstract
typeproperty with the@SerialName("\$type")annotation is correctly implemented. This setup is crucial for proper polymorphic serialization and deserialization of theFeedDefsReasonUnionsubclasses.
1-18: Overall: Well-implemented polymorphic base class for feed reasons.The
FeedDefsReasonUnionclass is well-structured and aligns with the PR objectives. It provides a solid foundation for representing different types of feed reasons, including the newreasonPintype. The use of a custom polymorphic serializer and type-safe casting properties enhances its functionality and ease of use.This implementation allows for easy extension to support additional reason types in the future, making it a flexible and maintainable solution.
core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/actor/ActorUpdateProfileRequest.kt (3)
4-4: LGTM: Import statement for RepoStrongRef added correctly.The new import for
RepoStrongRefis necessary and correctly placed to support the newpinnedPostproperty.
14-14: Good: Added trailing comma to clearBanner property.Adding a trailing comma to the
clearBannerproperty is a good practice in Kotlin. It improves readability when properties are listed vertically and makes future additions easier with cleaner diffs.
Line range hint
1-18: Overall: Excellent implementation of pinned post support.The changes to
ActorUpdateProfileRequestsuccessfully implement support for pinned posts as outlined in the PR objectives. The new properties and import are well-integrated into the existing class structure, following Kotlin best practices and maintaining consistency with the codebase.Key points:
- Proper import of
RepoStrongRefadded.- New properties
pinnedPostandclearPinnedPostcorrectly implemented.- Trailing comma added to improve code style and future maintainability.
These changes enhance the functionality of the class by enabling the management of a user's pinned post alongside their profile updates, which aligns perfectly with the PR's goals.
core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/PinnedPostSerializer.kt (3)
1-8: LGTM: Package declaration and imports are correct and complete.The package declaration is consistent with the file path, and all necessary imports for JSON serialization and the
RepoStrongRefmodel are present. There are no unused imports.
9-12: LGTM: Object declaration aligns with PR objectives.The
PinnedPostSerializerobject is correctly declared as a singleton and extendsJsonTransformingSerializer<RepoStrongRef>. This implementation aligns with the PR objective of supporting pinned posts, as it provides a custom serializer for theRepoStrongReftype, which is likely used to represent pinned posts.
1-24: Excellent implementation of PinnedPostSerializerThis implementation of
PinnedPostSerializereffectively addresses the PR objective of supporting pinned posts. Key points:
- It handles both current and legacy formats of pinned posts, ensuring backward compatibility.
- The code is concise, well-structured, and easy to understand.
- The use of
JsonTransformingSerializerprovides a clean way to customize the deserialization process.The serializer will work seamlessly with the
pinnedPostfeature inActorDefsProfileViewDetailedmentioned in the PR objectives. Great job on creating a robust and flexible solution for handling pinned posts!core/src/commonMain/kotlin/work/socialhub/kbsky/api/entity/app/bsky/feed/FeedGetAuthorFeedRequest.kt (2)
24-25: LGTM: New propertyincludePinsadded correctly.The new
includePinsproperty is well-implemented:
- Its purpose is clear from the name.
- The default value of
falsemaintains backward compatibility.- Making it non-nullable ensures type safety.
- Its placement in the class is logical, grouped with other request parameters.
This addition aligns well with the PR objective of supporting pinned posts in the author feed.
32-32: LGTM:toMap()method updated correctly.The
toMap()method has been properly updated to include the newincludePinsproperty:
- The property is added to the map using the
addParammethod, consistent with other properties.- The key "includePins" matches the property name, maintaining clarity and consistency.
This change ensures that the
includePinsparameter will be correctly included in API requests.core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/actor/ActorDefsProfileViewDetailed.kt (2)
5-6: LGTM: Import statements are correct and necessary.The new import statements for
RepoStrongRefandPinnedPostSerializerare correctly added and are essential for the newpinnedPostproperty and its custom serialization.
Line range hint
1-30: Summary: Changes align well with PR objectives and are well-implemented.The addition of the
pinnedPostproperty toActorDefsProfileViewDetailedsuccessfully implements the pinned post feature as described in the PR objectives. The use of a custom serializer (PinnedPostSerializer) effectively addresses the variation inpinnedPostformat used by some clients, particularly in Japan.The implementation is clean, well-documented, and aligns perfectly with the stated goals of the pull request. Great job on this update!
core/src/commonMain/kotlin/work/socialhub/kbsky/util/json/FeedDefsReasonPolymorphicSerializer.kt (3)
1-11: LGTM: Package declaration and imports are correct and appropriate.The package declaration follows Kotlin naming conventions, and all necessary imports are present without any unused ones.
12-15: LGTM:FeedDefsReasonPolymorphicSerializerobject is correctly implemented.The object declaration and its extension of
JsonContentPolymorphicSerializer<FeedDefsReasonUnion>are appropriate for implementing a polymorphic serializer for different feed reason types.
1-34: Overall assessment: Well-implemented polymorphic serializer supporting PR objectives.This new file successfully implements a polymorphic serializer for
FeedDefsReasonUnion, including support for the newFeedDefsReasonPintype as required by the PR objectives. The code is well-structured, follows Kotlin conventions, and provides a robust solution for handling different feed reason types.Minor suggestions for improvement include:
- Using a proper logging framework instead of
println.- Making the
Unknownclass more flexible by allowing the type to be set in the constructor.These changes would enhance the production-readiness and future-proofing of the code.
core/src/commonMain/kotlin/work/socialhub/kbsky/internal/app/bsky/_ActorResource.kt (3)
77-77: Improved null safety by removing forced null assertionThe removal of the
!!operator fromrequest.didis a positive change. It aligns with Kotlin's null safety features and reduces the risk of null pointer exceptions. This change suggests that thedidproperty is now properly handled as potentially nullable.
108-108: Consistent improvement in null safetyThis change is consistent with the earlier modification in line 77, further improving null safety by removing the forced null assertion (
!!) onrequest.did. This consistency in handling potentially nullable values throughout the function is commendable.
Line range hint
1-158: Overall assessment: Well-implemented changes with minor improvement suggestionThe changes in this file successfully implement the pinned post functionality as described in the PR objectives. The null safety improvements are consistent and commendable. The only suggestion for improvement is to add validation for the
pinnedPostvalue to ensure data integrity.Great job on implementing these features while maintaining code quality!
| val asReasonRepost get() = this as? FeedDefsReasonRepost | ||
| val asReasonPin get() = this as? FeedDefsReasonPin |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Convenient type-safe casting properties.
The asReasonRepost and asReasonPin extension properties provide a safe and convenient way to cast the FeedDefsReasonUnion to its specific subtypes.
Consider adding null checks or using the Elvis operator to provide default behavior when the cast fails. For example:
val asReasonRepost get() = this as? FeedDefsReasonRepost ?: throw IllegalStateException("Not a ReasonRepost")
val asReasonPin get() = this as? FeedDefsReasonPin ?: throw IllegalStateException("Not a ReasonPin")This approach would make it clearer when a cast fails and prevent potential null pointer exceptions further down the line.
There was a problem hiding this comment.
これらのプロパティを 非 null にすることは実用上あまり意味がありません。
post.reason?.asReasonRepost?.by のように使いますが、reason が nullable なので asReasonRepost が 非 null になっても post.reason?.asReasonRepost?.by と記述することに変わりないためです。
また、kbsky 内の他の asXxx プロパティも同様に nullable なのでここだけ 非 null になり例外が出るように変えることはむしろ混乱します。
There was a problem hiding this comment.
@takke、ご説明ありがとうございます。おっしゃるとおり、asXxx プロパティを nullable にすることで、一貫性が保たれますね。理解しました。
🐰🤔
✏️ Learnings added
Learnt from: takke
PR: uakihir0/kbsky#86
File: core/src/commonMain/kotlin/work/socialhub/kbsky/model/app/bsky/feed/FeedDefsReasonUnion.kt:16-17
Timestamp: 2024-09-28T10:32:00.259Z
Learning: In the kbsky library, `asXxx` properties are nullable, and making them non-nullable and throwing exceptions would cause confusion. Therefore, `asReasonRepost` and `asReasonPin` should remain nullable for consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
ポストの固定に対応しました
pinnedPosttoActorDefsProfileViewDetailedwith fallbackspinnedPostは形式が違うので落ちないようにカスタムシリアライザを追加しています。FeedDefsFeedViewPost.reasontoFeedDefsReasonUnionfromFeedDefsReasonRepostto supportreasonPinincludePinsparameter togetAuthorFeedpost.viewer.pinnedflagSummary by CodeRabbit
Release Notes
New Features
Enhancements
Bug Fixes
These changes enhance user experience by providing more control over content visibility and management.