Refactored Account to improve types & reduce boilerplate #566
Conversation
WalkthroughThis change introduces a comprehensive refactor of the Possibly related PRs
Suggested labels
✨ Finishing Touches
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. 🪧 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: 4
🔭 Outside diff range comments (1)
src/account/account.entity.ts (1)
88-108:⚠️ Potential issueIncorrect URL construction leads to duplicated “.ghost” segment
When
this.apIdalready contains/.ghost/activitypub/users/index, using a relative spec'.ghost/activitypub/${type}/${post.uuid}'results in
…/.ghost/activitypub/users/.ghost/activitypub/<type>/<uuid>(note the duplicated path element).
Pre-pending a leading slash makes the path absolute and eliminates the nesting.- return new URL(`.ghost/activitypub/${type}/${post.uuid}`, this.apId); + return new URL(`/.ghost/activitypub/${type}/${post.uuid}`, this.apId);
🧹 Nitpick comments (3)
src/account/account.repository.knex.integration.test.ts (1)
200-202: Improved test approach by using repository methodThe test now retrieves an actual entity through the repository instead of manually constructing one. This better tests the real behavior of the system.
However, consider creating a more complete Site object for clarity:
-const accountEntity = await accountRepository.getBySite({ - id: 1, -} as Site); +const accountEntity = await accountRepository.getBySite({ + id: 1, + host: 'testing.com', + webhook_secret: 'test-secret' +});src/account/account.entity.ts (2)
26-37:urlis non-nullable onAccountDraftbut nullable in draft input
AccountDraft.urlis declared asURL(non-nullable), yet bothInternalAccountDraftDataandExternalAccountDraftDataallowurl: URL | null.
draft()patches this by replacing anullwithapId, but the discrepancy can still confuse users of the types.Consider aligning the types so that
urlis consistently optional/nullable in the input and guaranteed non-null in the output:export interface AccountDraft { uuid: string; - url: URL; + url: URL; // guaranteed non-null in the result } // … type InternalAccountDraftData = { // … - url: URL | null; + url?: URL | null; };or document the “nullable-in, non-nullable-out” contract in comments.
54-68: Constructor proxy increate()is verbose
create()manually passes every property to the constructor, which is error-prone when new properties are added.
You can leverage parameter spreading to avoid the boilerplate:- return new AccountEntity( - data.id, - data.uuid, - data.username, - data.name, - data.bio, - data.url, - data.avatarUrl, - data.bannerImageUrl, - data.apId, - data.apFollowers, - data.isInternal, - ); + return new AccountEntity( + data.id, + data.uuid, + data.username, + data.name, + data.bio, + data.url, + data.avatarUrl, + data.bannerImageUrl, + data.apId, + data.apFollowers, + data.isInternal + );—or even—
return new AccountEntity(...Object.values(data) as ConstructorParameters<typeof AccountEntity>);to keep the method future-proof.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/account/account.entity.ts(2 hunks)src/account/account.entity.unit.test.ts(2 hunks)src/account/account.repository.knex.integration.test.ts(4 hunks)src/account/account.repository.knex.ts(3 hunks)src/account/account.service.ts(1 hunks)src/account/account.service.unit.test.ts(5 hunks)src/activitypub/fediverse-bridge.unit.test.ts(3 hunks)src/feed/feed-update.service.unit.test.ts(4 hunks)src/feed/feed.service.integration.test.ts(3 hunks)src/http/api/__snapshots__/post-authored-by-me.json(2 hunks)src/http/api/__snapshots__/post-liked-by-me.json(2 hunks)src/http/api/__snapshots__/post-reposted-by-me.json(2 hunks)src/http/api/__snapshots__/post.json(2 hunks)src/http/api/__snapshots__/thread.json(4 hunks)src/http/api/account.ts(3 hunks)src/http/api/feed.unit.test.ts(3 hunks)src/http/api/helpers/account.ts(0 hunks)src/http/api/helpers/account.unit.test.ts(0 hunks)src/http/api/helpers/post.unit.test.ts(3 hunks)src/http/api/post.unit.test.ts(3 hunks)src/http/api/thread.unit.test.ts(2 hunks)src/http/api/views/account.follows.view.integration.test.ts(3 hunks)src/http/api/views/account.follows.view.ts(3 hunks)src/http/api/views/account.posts.view.integration.test.ts(3 hunks)src/http/api/views/account.posts.view.ts(2 hunks)src/post/post.entity.ts(0 hunks)src/post/post.entity.unit.test.ts(1 hunks)src/post/post.repository.knex.integration.test.ts(1 hunks)src/post/post.repository.knex.ts(3 hunks)
💤 Files with no reviewable changes (3)
- src/post/post.entity.ts
- src/http/api/helpers/account.unit.test.ts
- src/http/api/helpers/account.ts
🧰 Additional context used
🧬 Code Graph Analysis (13)
src/activitypub/fediverse-bridge.unit.test.ts (1)
src/account/account.entity.ts (1)
AccountEntity(39-124)
src/post/post.entity.unit.test.ts (1)
src/account/account.entity.ts (2)
draft(70-86)AccountEntity(39-124)
src/http/api/views/account.posts.view.integration.test.ts (1)
src/account/account.entity.ts (1)
Account(4-24)
src/http/api/views/account.follows.view.integration.test.ts (1)
src/account/account.entity.ts (1)
Account(4-24)
src/account/account.service.unit.test.ts (1)
src/account/account.entity.ts (1)
AccountEntity(39-124)
src/http/api/thread.unit.test.ts (1)
src/account/account.entity.ts (2)
draft(70-86)AccountEntity(39-124)
src/http/api/feed.unit.test.ts (1)
src/account/account.entity.ts (2)
AccountEntity(39-124)draft(70-86)
src/http/api/views/account.posts.view.ts (1)
src/account/account.entity.ts (1)
Account(4-24)
src/http/api/post.unit.test.ts (1)
src/account/account.entity.ts (2)
AccountEntity(39-124)draft(70-86)
src/post/post.repository.knex.ts (2)
src/account/account.entity.ts (1)
AccountEntity(39-124)src/core/url.ts (1)
parseURL(1-7)
src/account/account.repository.knex.integration.test.ts (2)
src/account/account.entity.ts (1)
AccountEntity(39-124)src/site/site.service.ts (1)
Site(7-11)
src/account/account.entity.ts (1)
src/post/post.entity.ts (1)
CreatePostType(14-14)
src/account/account.repository.knex.ts (2)
src/account/account.entity.ts (1)
AccountEntity(39-124)src/core/url.ts (1)
parseURL(1-7)
🔇 Additional comments (65)
src/http/api/__snapshots__/post-reposted-by-me.json (5)
4-4: Approve updated avatarUrl
The protocol change to HTTP and the path adjustment from/avatars/to/avatar/align with the new URL conventions adopted in this refactor.
6-6: Approve updated author id
Replacing the previous HTTPS-based user ID with the explicit/.ghost/activitypub/users/indexpath correctly reflects the new ActivityPub URL pattern for author entities.
8-8: Approve updated author url
This change mirrors the author’sidupdate and conforms to the standardized HTTP ActivityPub URL format.
14-14: Approve updated post id
The nested/.ghost/activitypub/users/segment in the post’sidaccurately captures the new path structure introduced by the immutableAccountEntityrefactor.
29-29: No action needed
The closing JSON brace adjustment is purely structural and does not impact the snapshot’s semantics.src/activitypub/fediverse-bridge.unit.test.ts (3)
2-2: ReplacedAccountwithAccountEntityimport.The import of
Accounthas been replaced withAccountEntity, aligning with the PR's refactoring that makesAccountan interface instead of a concrete class.
38-38: Updated mock object creation to useAccountEntity.Mock author creation now uses
AccountEntityas the prototype object instead ofAccount, consistent with the refactoring that makesAccountan interface andAccountEntitythe implementing class.
83-83: Updated mock object creation to useAccountEntity.Similar to the previous instance, this mock author creation now uses
AccountEntityas the prototype object instead ofAccount.src/post/post.repository.knex.integration.test.ts (1)
512-513: Improved assertion style for immutable objects.Changed from using strict equality checks with
assert(x === y)to using value equality withassert.equal(x, y). This is a better practice when working with immutable objects, as it focuses on value comparison rather than reference equality.src/http/api/account.ts (3)
115-115: Removed explicit type assertion toPersistedAccount.The type assertion to
PersistedAccounthas been removed as part of the PR's refactoring, which eliminates the need for thePersistedAccounttype. TheAccountEntityinstances returned byaccountRepository.getBySite()now directly implement theAccountinterface.
257-257: Removed explicit type assertion toPersistedAccount.Similar to the previous instance, the type assertion has been removed, simplifying the code while maintaining type safety through the
Accountinterface.
279-279: Removed explicit type assertion toPersistedAccount.The type assertion for
getByApIdhas been removed, consistent with the overall refactoring approach that replaces thePersistedAccounttype with theAccountinterface.src/http/api/views/account.posts.view.integration.test.ts (4)
1-1: Updated import to useAccountinterface.Changed the import from
PersistedAccounttoAccount, reflecting the PR's goal of using theAccountinterface as the primary type throughout the codebase.
29-31: Updated type annotations for account variables.The type annotations for
siteDefaultAccountandaccountEntityhave been changed fromPersistedAccount | nulltoAccount | null, aligning with the new type system whereAccountis the primary interface.
84-87: Removed explicit type assertion in repository call.Removed the explicit type assertion to
PersistedAccountwhen retrieving an account by AP ID. The repository method now correctly returns anAccounttype without needing the cast.
92-95: Removed explicit type assertion in repository call.Similarly, removed the type assertion for the second repository call, maintaining consistency with the new type system that uses the
Accountinterface directly.src/post/post.entity.unit.test.ts (1)
3-3: Properly implemented the new immutable AccountEntity patternThe updated code correctly implements the new immutable account pattern using
AccountEntity.draft()andAccountEntity.create()factory methods. This approach aligns with the overall refactoring goal of using immutable entities with explicit construction rather than mutable objects.Also applies to: 7-33
src/account/account.service.ts (1)
545-547: Correctly implemented immutable entity update patternThe changes properly capture the new immutable entity returned by
updateProfileand pass it to the repository's save method. This aligns with the immutable entity pattern where methods return new instances rather than mutating the existing one.src/http/api/views/account.follows.view.integration.test.ts (1)
2-2: Type simplification through consistent Account interface usageThe changes correctly remove references to the
PersistedAccounttype, using theAccountinterface consistently throughout the test. This aligns with the refactoring goal of simplifying types by ensuring that theAccountentity always has a requiredidfield, eliminating the need for explicit type casting.Also applies to: 45-45, 137-139
src/account/account.service.unit.test.ts (1)
6-6: Tests properly updated to match immutable entity patternThe changes correctly update the tests to match the immutable entity pattern implemented in the service. The test now:
- Properly imports and uses the
AccountEntitytype- Creates a separate
updatedvariable to represent the new instance returned byupdateProfile- Mocks the
updateProfilemethod to return this new instance- Verifies that the repository's save method is called with the new instance
This ensures the test accurately validates the service's handling of immutable entities.
Also applies to: 38-41, 60-60, 79-79, 95-95
src/http/api/helpers/post.unit.test.ts (3)
3-3: Import updated to use the new AccountEntity class.The import is now correctly using the refactored immutable
AccountEntityclass instead of the previous mutableAccountclass, aligning with the broader refactoring effort.
7-23: Good abstraction with the createAuthor helper function.Creating a reusable helper function for author creation improves test maintainability and reduces duplication. The implementation properly follows the new two-step pattern with
AccountEntity.draft()followed byAccountEntity.create(), which ensures immutability.
27-27: Consistent use of the helper function throughout tests.All test cases now use the centralized
createAuthor()function, maintaining consistency and making future changes easier to implement.Also applies to: 40-40, 54-54
src/http/api/views/account.posts.view.ts (2)
7-7: Updated import to use the new Account interface.The import now uses the
Accountinterface type instead of the previousPersistedAccounttype, which aligns with the refactoring goals of simplifying type checking.
85-86: Method signature updated to use Account interface.The
getPostsByApIdmethod signature has been updated to use:
account: Account | nullinstead ofPersistedAccountcurrentContextAccount: Accountinstead ofPersistedAccountThis change properly implements the PR objective to remove the need for ID presence checks and type casting since the
Accountinterface now guarantees thatidis always present.src/http/api/__snapshots__/thread.json (1)
6-6: Updated URL formats in test snapshot.The URL formats have been updated to match the refactored account entity implementation:
- Avatar URLs now use HTTP protocol and a simplified path format (
/avatar/instead of/avatars/)- Account IDs and URLs now include the ActivityPub path prefix (
/.ghost/activitypub/users/index)- Post IDs now include the nested ActivityPub path structure
These changes correctly reflect the URL generation logic in the refactored
AccountEntityclass and ensure test snapshots remain valid.Also applies to: 8-8, 10-10, 16-16, 35-35, 37-37, 39-39, 45-45, 64-64, 66-66, 68-68, 74-74
src/http/api/post.unit.test.ts (3)
3-3: Updated import to use the new AccountEntity class.The import now correctly uses the refactored immutable
AccountEntityclass instead of the previous mutableAccountclass.
14-14: Updated account type to use AccountEntity.The account variable type is updated to
AccountEntityto match the refactored immutable entity class.
76-89: Refactored account creation to use the immutable pattern.The account creation logic has been updated to use the new two-step pattern for creating immutable account entities:
- First create a draft with
AccountEntity.draft()including the necessary properties- Then create the final entity with
AccountEntity.create()with an explicit IDThis correctly implements the immutable entity pattern introduced in the PR and ensures type safety without needing to check for ID presence.
src/http/api/__snapshots__/post-liked-by-me.json (2)
4-8: URL pattern changes in the snapshot test dataThe changes to the avatar, id, and url fields reflect the new URL pattern used by the refactored
AccountEntity. The protocol changed from HTTPS to HTTP and the path structure now follows the ActivityPub URL convention with the.ghost/activitypub/users/indexpattern.
14-14: Updated post ID URL structureThe post ID URL now includes additional path segments with a nested structure that includes both the user path and article path. This change aligns with the refactored
AccountEntity.getApIdForPost()method implementation.src/http/api/thread.unit.test.ts (2)
3-3: Updated import to use type Account and AccountEntityThe import statement has been updated to import the
AccountEntityclass andAccountas a type-only import, which aligns with the refactored immutable account model.
25-38: Refactored account creation using immutable patternThe account creation has been refactored to use the new two-step pattern:
- First creating a draft using
AccountEntity.draft()with necessary properties- Then creating the final entity with
AccountEntity.create()by adding an IDThis correctly implements the immutable entity pattern introduced in the PR, where entities are created explicitly rather than mutated after creation.
src/http/api/feed.unit.test.ts (3)
3-3: Updated import to use AccountEntityThe import statement has been updated to import the
AccountEntityclass, aligning with the refactored immutable account model.
15-15: Updated account type annotationThe type annotation has been updated to explicitly use
AccountEntityinstead of the interface type, which makes the code more precise about the expected implementation.
25-38: Refactored account creation using immutable patternSimilar to the thread test, the account creation has been refactored to use the new two-step pattern with
AccountEntity.draft()andAccountEntity.create(), correctly implementing the immutable entity pattern.src/feed/feed.service.integration.test.ts (3)
7-7: Updated import to use AccountEntity typeThe import has been updated to import
AccountEntityas a type for use in the function signatures, aligning with the refactored account model.
73-76: Updated parameter type for createPost functionThe parameter type has been updated from
AccounttoAccountEntityto properly type-check against the refactored immutable account entity.
98-98: Updated parameter type for getFeedDataForAccount functionThe parameter type has been updated from
AccounttoAccountEntityto maintain type consistency throughout the codebase.src/http/api/__snapshots__/post-authored-by-me.json (2)
4-8: URL structure updated to match new ActivityPub conventionsThe author's URLs have been changed to use HTTP protocol and follow the ActivityPub URL structure with the
.ghost/activitypub/users/path. These changes align with the account entity refactoring to use standardized URL formats.
14-14: Post ID URL updated to new formatThe post ID has been updated to a longer format that includes the ActivityPub path prefix. This standardizes how resources are identified in the system.
src/feed/feed-update.service.unit.test.ts (3)
6-6: Import updated to use new AccountEntityThe import statement has been updated to use the new immutable AccountEntity class instead of the previous Account class.
20-46: Account creation refactored to use immutable patternThe account creation has been updated to use the new two-step process: first creating a draft with
AccountEntity.draft()and then creating the full entity withAccountEntity.create(). This follows the new immutable pattern where an account always has a required ID.The code now explicitly sets
isInternal: trueand uses proper URL objects instead of relying on internal site logic.
184-200: Blocked account creation updated to match new patternSimilar to the main account creation, the blocked account is now created using the two-step draft and create process. This ensures consistency in how accounts are instantiated throughout the codebase.
src/account/account.entity.unit.test.ts (4)
3-5: Import and describe updated for new entity nameThe import and test suite description have been updated to use AccountEntity instead of Account.
7-22: Account creation updated to use two-step processThe test now uses the new immutable pattern with
AccountEntity.draft()andAccountEntity.create()factory methods instead of directly constructing an Account instance. This aligns with the refactoring goal of making the entity immutable with a required ID.
91-98: UpdateProfile now returns a new instance instead of mutatingThe test has been updated to capture the return value from
updateProfile()and assert against it, rather than expecting mutations on the original object. This change verifies the immutability pattern where update operations return new instances.
298-309: Null value handling with immutable updatesThe test for setting values to null now correctly verifies that the returned instance has null values while keeping other properties unchanged. This ensures the immutable update pattern works correctly with null values.
src/http/api/__snapshots__/post.json (2)
4-8: Author URLs updated to ActivityPub formatThe author's avatar, ID, and URL have been updated to use HTTP protocol and the standardized ActivityPub path structure. This ensures consistent URL handling across the application.
14-14: Post ID URL standardized to include ActivityPub pathThe post ID URL now follows the standardized format with the ActivityPub path prefix, consistent with other entities in the system.
src/account/account.repository.knex.integration.test.ts (6)
11-11: Type import added for better type safetyThe explicit import of
Sitetype alongside the service class improves type clarity in the code.
13-13: Updated import to use new AccountEntity classThis change aligns with the refactoring to use the immutable AccountEntity model.
63-63: Updated assertion to check for AccountEntity instanceThe assertion now correctly verifies the returned object is an instance of AccountEntity.
204-207: Using immutable update pattern correctlyThe code now uses the immutable pattern where
updateProfilereturns a new instance rather than mutating the existing one.
210-210: Saving the updated entityCorrectly saves the new entity instance returned from updateProfile.
220-220: Updated event assertionThe assertion now correctly verifies the updated entity is included in the event.
src/http/api/views/account.follows.view.ts (3)
2-2: Simplified type importImport changed to use the simpler
Accounttype, removing the need for thePersistedAccounttype and explicit type checks.
46-51: Simplified parameter typeThe parameter type was changed from
PersistedAccounttoAccount, matching the PR objective of simplifying type checking by making the Account entity always have a requiredidfield. This removes the need for existence checks on the ID property.
93-98: Simplified parameter typeSimilarly updated
siteDefaultAccountparameter type toAccount, maintaining consistency throughout the codebase.src/post/post.repository.knex.ts (1)
5-5: Updated import to use AccountEntityChanged import to use the new immutable
AccountEntityclass.src/account/account.repository.knex.ts (5)
7-7: Updated import for new entity modelImport changed to use both the
Accounttype and theAccountEntityclass implementation.
15-30: Simplified save methodThe save method no longer needs to check for new accounts, aligning with the immutable entity pattern where all entities are considered valid with required properties.
46-59: Improved variable naming and UUID generationRenamed
accounttoaccountRowto better reflect that this is a database row, not an entity. Also updated the UUID generation to modify the row before updating the database.
61-73: Using factory method for entity creationNow properly using
AccountEntity.createto instantiate immutable entities from database rows. This matches the new pattern where all properties are set at creation time.
107-119: Proper immutable entity creationCorrectly using
AccountEntity.createfor entity instantiation, with theisInternalflag based onsite_idpresence.
| updateProfile(params: ProfileUpdateParams) { | ||
| type P = ProfileUpdateParams; | ||
| const get = <K extends keyof P>(prop: K): P[K] => | ||
| params[prop] === undefined ? this[prop] : params[prop]; | ||
|
|
||
| return AccountEntity.create({ | ||
| ...this, | ||
| username: get('username'), | ||
| name: get('name'), | ||
| bio: get('bio'), | ||
| avatarUrl: get('avatarUrl'), | ||
| bannerImageUrl: get('bannerImageUrl'), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updateProfile can assign undefined to non-optional fields
get('username') (and the other calls) returns string | undefined, but username in Data<Account> is required (string).
With strictNullChecks enabled this does not type-check and at runtime may propagate an undefined username.
A safer, fully-typed alternative:
- type P = ProfileUpdateParams;
- const get = <K extends keyof P>(prop: K): P[K] =>
- params[prop] === undefined ? this[prop] : params[prop];
-
- return AccountEntity.create({
- ...this,
- username: get('username'),
- name: get('name'),
- bio: get('bio'),
- avatarUrl: get('avatarUrl'),
- bannerImageUrl: get('bannerImageUrl'),
- });
+ return AccountEntity.create({
+ ...this,
+ username: params.username ?? this.username,
+ name: params.name ?? this.name,
+ bio: params.bio ?? this.bio,
+ avatarUrl: params.avatarUrl ?? this.avatarUrl,
+ bannerImageUrl: params.bannerImageUrl ?? this.bannerImageUrl,
+ });This keeps the implementation concise, avoids the helper closure, and preserves correct types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updateProfile(params: ProfileUpdateParams) { | |
| type P = ProfileUpdateParams; | |
| const get = <K extends keyof P>(prop: K): P[K] => | |
| params[prop] === undefined ? this[prop] : params[prop]; | |
| return AccountEntity.create({ | |
| ...this, | |
| username: get('username'), | |
| name: get('name'), | |
| bio: get('bio'), | |
| avatarUrl: get('avatarUrl'), | |
| bannerImageUrl: get('bannerImageUrl'), | |
| }); | |
| } | |
| updateProfile(params: ProfileUpdateParams) { | |
| return AccountEntity.create({ | |
| ...this, | |
| username: params.username ?? this.username, | |
| name: params.name ?? this.name, | |
| bio: params.bio ?? this.bio, | |
| avatarUrl: params.avatarUrl ?? this.avatarUrl, | |
| bannerImageUrl: params.bannerImageUrl ?? this.bannerImageUrl, | |
| }); | |
| } |
There was a problem hiding this comment.
I don't think this is true, we explicitly do not return undefined if it is that on the params object, and instead return the prop from this which does match the type required in Data<Account>
There was a problem hiding this comment.
@allouis You're absolutely right. Looking closer at your implementation, the runtime behavior is perfectly safe:
const get = <K extends keyof P>(prop: K): P[K] =>
params[prop] === undefined ? this[prop] : params[prop];This correctly falls back to this[prop] when params[prop] is undefined, so it will never return undefined for required fields.
The only subtlety is that TypeScript's type system declares the return type as P[K] which includes undefined for optional properties, so static type checking might complain even though the runtime behavior is correct.
Thanks for the clarification and pushing back on my suggestion!
✏️ Learnings added
Learnt from: allouis
PR: TryGhost/ActivityPub#566
File: src/account/account.entity.ts:110-123
Timestamp: 2025-04-24T10:41:15.039Z
Learning: In the Account entity's `updateProfile` method, the current implementation using a helper function that checks for undefined values is type-safe at runtime, even though TypeScript might not perfectly infer this due to the function's return type declaration.
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.
| */ | ||
| updateProfile(params: ProfileUpdateParams): Account; | ||
| /** | ||
| * @deprecated |
There was a problem hiding this comment.
This code is weird and shouldn't really live here, or at least not be exposed like this - why does an Account have a getApIdForPost method lol
| readonly avatarUrl: URL | null; | ||
| readonly bannerImageUrl: URL | null; | ||
| readonly apId: URL; | ||
| readonly apFollowers: URL | null; |
There was a problem hiding this comment.
This is nullable now
There was a problem hiding this comment.
Should this be null though? When would an account never have a followers URL?
There was a problem hiding this comment.
Great question, I don't know - but it's nullable in the DB, so I think we should fix that all together.
| ); | ||
| updateProfile(params: ProfileUpdateParams) { | ||
| type P = ProfileUpdateParams; | ||
| const get = <K extends keyof P>(prop: K): P[K] => |
There was a problem hiding this comment.
I just wanted to avoid the repetition of
username: params.username === undefined ? this.username : params.username,But I can put it back if it's preferred
There was a problem hiding this comment.
Nah, this is a cool utility, maybe we have generic utils module thing somewhere?
There was a problem hiding this comment.
I did try to pull it out - but unfortunately it does't work well when you pull it out, because you need to pass in the this value and it becomes a bit of a mess like get(this, params, 'key') PLUS the types become super fucky
src/account/account.entity.ts
Outdated
| type AccountDraftData = InternalAccountDraftData | ExternalAccountDraftData; | ||
|
|
||
| type Data<T> = { | ||
| // biome-ignore lint/suspicious/noExplicitAny: These are not publicly usable instances of any |
There was a problem hiding this comment.
This is legit - the any here is just to match functions, which we strip out - it doesn't leak into our code
There was a problem hiding this comment.
Maybe update the message to include this?
|
|
||
| type AccountDraftData = InternalAccountDraftData | ExternalAccountDraftData; | ||
|
|
||
| type Data<T> = { |
There was a problem hiding this comment.
Yeah I can probably put this somewhere else, but where?
| } as Site); | ||
|
|
||
| accountEntity.updateProfile({ | ||
| const updated = accountEntity.updateProfile({ |
There was a problem hiding this comment.
Immutable - so this had to change
| ) {} | ||
|
|
||
| async save(account: Account): Promise<void> { | ||
| if (account.isNew) { |
There was a problem hiding this comment.
Accounts are never new!
| apId: URL, | ||
| account: PersistedAccount, | ||
| currentContextAccount: PersistedAccount, | ||
| account: Account | null, |
There was a problem hiding this comment.
No more PersistedAccount! But this is nullable, I needed to fix the type. We do account?.isInternal below - so this is safe
| } | ||
|
|
||
| addRepost(account: Account) { | ||
| if (!account.id) { |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/account/account.repository.knex.ts (1)
5-5: Consider removing unused Site importThe
Sitetype is still being imported, but since the PR mentions removing the internal site property from the entity in favor of theisInternalflag, this import might be unnecessary unless it's used elsewhere in the file.-import type { Site } from '../site/site.service';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/account/account.repository.knex.ts(2 hunks)src/post/post.repository.knex.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/post/post.repository.knex.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/account/account.repository.knex.ts (2)
src/account/account.entity.ts (2)
AccountEntity(39-124)Account(4-24)src/core/url.ts (1)
parseURL(1-7)
🔇 Additional comments (5)
src/account/account.repository.knex.ts (5)
7-7: Import change to support the Account refactoringThe import statement correctly switches from the previous
Accountclass to importing both theAccountinterface type andAccountEntityimplementation class, aligning with the PR's goal of making accounts immutable.
46-59: Database query simplified by removing site-related joinsThe query has been appropriately simplified to only select account fields, removing any site-related joins that were needed for the previous mutable implementation.
73-85: Account creation refactored to use factory methodThe implementation now correctly uses
AccountEntity.createfactory method to create immutable accounts, with explicitisInternal: truefor site-based accounts, which is cleaner than the previous approach using site objects.
94-94: Added uuid column to SELECT queryThis change addresses the previous review comment about missing uuid in the SELECT query, which was causing unnecessary UUID regeneration and updates.
118-130: Improved account creation with explicit isInternal flagThe implementation now uses the
AccountEntity.createfactory method withisInternalflag based on the presence of a site_id, replacing the mutable approach. This better aligns with the PR objective of simplifying type checking for persisted accounts.
allouis
force-pushed
the
account-entity-block-unblock
branch
from
4490ef6 to
280032d
Compare
allouis
force-pushed
the
account-entity-block-unblock
branch
from
280032d to
232436d
Compare
| readonly avatarUrl: URL | null; | ||
| readonly bannerImageUrl: URL | null; | ||
| readonly apId: URL; | ||
| readonly apFollowers: URL | null; |
There was a problem hiding this comment.
Should this be null though? When would an account never have a followers URL?
src/account/account.entity.ts
Outdated
| type AccountDraftData = InternalAccountDraftData | ExternalAccountDraftData; | ||
|
|
||
| type Data<T> = { | ||
| // biome-ignore lint/suspicious/noExplicitAny: These are not publicly usable instances of any |
There was a problem hiding this comment.
Maybe update the message to include this?
| ); | ||
| updateProfile(params: ProfileUpdateParams) { | ||
| type P = ProfileUpdateParams; | ||
| const get = <K extends keyof P>(prop: K): P[K] => |
There was a problem hiding this comment.
Nah, this is a cool utility, maybe we have generic utils module thing somewhere?
| export interface AccountData { | ||
| id: number; | ||
| uuid: string | null; | ||
| export interface AccountDraft { |
There was a problem hiding this comment.
What about this just being type AccountDraft = Omit<Account, 'id'> so we don't have to list all of the account properties here?
There was a problem hiding this comment.
Yeah so - my thinking here is that these two types at the top of the file serve as documentation, and the duplication is preferred because it's easier to read right away. What do you think about that?
There was a problem hiding this comment.
Ah ok, yeh i'm fine with that, i guess it's unlikely they are going to come out of sync without us knowing
ref 959010f Since we've moved to using an AccountView which bypasses the entity layer completely when reading accounts we are not using these helpers.
ref https://linear.app/ghost/issue/AP-1082 The Account entity had grown complex due to its implementation with private members and public getters for privately mutable state. We were also encountering issues with nullable `id` fields that required awkward PersistedAccount type workarounds. - Account now always has a required `id` field - Introduced AccountDraft type for creating new accounts without IDs - Made AccountEntity immutable, returning new instances on save operations - Aligned `apFollowers` field to be nullable, matching database schema This refactoring simplifies type checking in the block method implementation, eliminating the need for ID checking and type casting when ensuring we're working with persisted accounts. No functional changes were introduced, though there is a slight type change with `apFollowers` now being nullable to match the database.
allouis
force-pushed
the
account-entity-block-unblock
branch
from
c132064 to
c7cdeb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/http/api/__snapshots__/post-liked-by-me.json (1)
29-29: Restore trailing newline in snapshot file
Most JSON snapshot tools expect a newline at the end of the file. Re-adding it helps prevent inconsequential diffs in future snapshot updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/account/account.entity.ts(2 hunks)src/account/account.entity.unit.test.ts(2 hunks)src/account/account.repository.knex.integration.test.ts(5 hunks)src/account/account.repository.knex.ts(2 hunks)src/account/account.service.ts(1 hunks)src/account/account.service.unit.test.ts(5 hunks)src/activitypub/fediverse-bridge.unit.test.ts(3 hunks)src/feed/feed-update.service.unit.test.ts(4 hunks)src/feed/feed.service.integration.test.ts(3 hunks)src/http/api/__snapshots__/post-authored-by-me.json(2 hunks)src/http/api/__snapshots__/post-liked-by-me.json(2 hunks)src/http/api/__snapshots__/post-reposted-by-me.json(2 hunks)src/http/api/__snapshots__/post.json(2 hunks)src/http/api/__snapshots__/thread.json(4 hunks)src/http/api/account.ts(3 hunks)src/http/api/feed.unit.test.ts(3 hunks)src/http/api/helpers/account.ts(0 hunks)src/http/api/helpers/account.unit.test.ts(0 hunks)src/http/api/helpers/post.unit.test.ts(3 hunks)src/http/api/post.unit.test.ts(3 hunks)src/http/api/thread.unit.test.ts(2 hunks)src/http/api/views/account.follows.view.integration.test.ts(3 hunks)src/http/api/views/account.follows.view.ts(3 hunks)src/http/api/views/account.posts.view.integration.test.ts(3 hunks)src/http/api/views/account.posts.view.ts(2 hunks)src/post/post.entity.ts(0 hunks)src/post/post.entity.unit.test.ts(1 hunks)src/post/post.repository.knex.integration.test.ts(1 hunks)src/post/post.repository.knex.ts(5 hunks)
💤 Files with no reviewable changes (3)
- src/post/post.entity.ts
- src/http/api/helpers/account.unit.test.ts
- src/http/api/helpers/account.ts
✅ Files skipped from review due to trivial changes (2)
- src/http/api/snapshots/thread.json
- src/feed/feed.service.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- src/activitypub/fediverse-bridge.unit.test.ts
- src/post/post.entity.unit.test.ts
- src/http/api/snapshots/post-reposted-by-me.json
- src/http/api/account.ts
- src/http/api/helpers/post.unit.test.ts
- src/feed/feed-update.service.unit.test.ts
- src/account/account.service.ts
- src/account/account.service.unit.test.ts
- src/http/api/views/account.posts.view.ts
- src/http/api/snapshots/post.json
- src/http/api/feed.unit.test.ts
- src/http/api/views/account.posts.view.integration.test.ts
- src/account/account.repository.knex.integration.test.ts
- src/http/api/thread.unit.test.ts
- src/http/api/snapshots/post-authored-by-me.json
- src/post/post.repository.knex.integration.test.ts
- src/account/account.entity.unit.test.ts
- src/http/api/views/account.follows.view.integration.test.ts
- src/post/post.repository.knex.ts
- src/account/account.entity.ts
- src/http/api/post.unit.test.ts
- src/http/api/views/account.follows.view.ts
- src/account/account.repository.knex.ts
🔇 Additional comments (2)
src/http/api/__snapshots__/post-liked-by-me.json (2)
4-4: Confirm updated avatar URL path
The change from"avatars/foobar.png"to"avatar/foobar.png"reflects the new URL pattern. Please verify that the underlying code generatingavatarUrlwas updated to use the singularavatarsegment and that no endpoints or clients expect the pluralavatars.
6-8: Verify author ID and URL consistency
Both theidandurlfields are now"http://example.com/.ghost/activitypub/users/index". Ensure this aligns with the newAccountEntitydesign (isInternalvs. external URL handling) and thatidandurlshould indeed be identical for internal actor endpoints.
ref https://linear.app/ghost/issue/AP-1082
The Account entity had grown complex due to its implementation with private
members and public getters for privately mutable state. We were also
encountering issues with nullable
idfields that required awkwardPersistedAccount type workarounds.
idfieldapFollowersfield to be nullable, matching database schemaThis refactoring simplifies type checking in the block method implementation,
eliminating the need for ID checking and type casting when ensuring we're
working with persisted accounts. No functional changes were introduced, though
there is a slight type change with
apFollowersnow being nullable to matchthe database.