Fixed social web handle not being constructed from API in Notes/Reader#28477
Merged
Conversation
Contributor
WalkthroughThis pull request replaces the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
ref TryGhost/ActivityPub#1821 In TryGhost/ActivityPub#1821, we're introducing support for changing the displayed domain on a social web handle to a different subdomain or another domain they own. For example, a site hosted on jamie.larson.com can choose to expose their handle as @jamie@larson.com (instead of @index@jamie.larson.com). In Notes/Reader, we were re-constructing the displayed handle from the ap_id, which meant we did not correctly displayed the new handle returned by the API. Two additional tidy-ings: - renamed getUsername to getHandle - simplified calls such as `actor.handle || getHandle(actor)` to `getHandle(actor)`
sagzy
force-pushed
the
ap-display-correct-handle
branch
from
8f34531 to
2cd9eca
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f34531745
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/activitypub/src/utils/get-handle.ts (1)
1-1: 💤 Low valueConsider adding spaces around the union type operator.
The type annotation
id: string|nullis more commonly written asid: string | nullin TypeScript for better readability.♻️ Suggested formatting improvement
-function getHandle(actor: {handle?: string; preferredUsername: string; id: string|null;}) { +function getHandle(actor: {handle?: string; preferredUsername: string; id: string | null;}) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/activitypub/src/utils/get-handle.ts` at line 1, The type annotation in the getHandle function uses `string|null`; update it to the more readable `string | null` by adding spaces around the union operator in the parameter type (actor: { handle?: string; preferredUsername: string; id: string | null; }) so it follows TypeScript style conventions; ensure any other occurrences in the same file follow the same spacing for consistency.apps/activitypub/test/unit/utils/get-handle.test.tsx (1)
4-4: 💤 Low valueRemove unnecessary
asyncdeclarations from test functions.These test functions don't contain any
awaitstatements or async operations. Theasynckeyword can be removed for clarity.♻️ Suggested simplification
- it('returns the API-provided handle when present', async function () { + it('returns the API-provided handle when present', function () {Apply the same change to all four test functions (lines 4, 16, 27, 38).
Also applies to: 16-16, 27-27, 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/activitypub/test/unit/utils/get-handle.test.tsx` at line 4, Remove the unnecessary async keyword from the four test functions in get-handle.test.tsx; locate the four it(...) blocks with descriptions "returns the API-provided handle when present", "returns the API-provided handle when not present" (if present), and the other two test descriptions on lines noted in the review and simply delete the async modifier from their function declarations so they are plain synchronous functions (e.g., change "it('returns the API-provided handle when present', async function () {" to "it('returns the API-provided handle when present', function () {"). Ensure you update all four it(...) blocks referenced in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/activitypub/src/utils/posts.ts`:
- Around line 26-27: The preferredUsername assignments derived from
post.author.handle and post.repostedBy.handle assume an `@user`@domain shape and
can produce undefined; update both occurrences (the preferredUsername lines
using post.author.handle.split('@')[1] and post.repostedBy.handle.split('@')[1])
to provide a safe fallback (e.g. use nullish coalescing to default to 'unknown'
or a sensible fallback) so an unexpected handle format never propagates
undefined into the ActivityPub actor.
---
Nitpick comments:
In `@apps/activitypub/src/utils/get-handle.ts`:
- Line 1: The type annotation in the getHandle function uses `string|null`;
update it to the more readable `string | null` by adding spaces around the union
operator in the parameter type (actor: { handle?: string; preferredUsername:
string; id: string | null; }) so it follows TypeScript style conventions; ensure
any other occurrences in the same file follow the same spacing for consistency.
In `@apps/activitypub/test/unit/utils/get-handle.test.tsx`:
- Line 4: Remove the unnecessary async keyword from the four test functions in
get-handle.test.tsx; locate the four it(...) blocks with descriptions "returns
the API-provided handle when present", "returns the API-provided handle when not
present" (if present), and the other two test descriptions on lines noted in the
review and simply delete the async modifier from their function declarations so
they are plain synchronous functions (e.g., change "it('returns the API-provided
handle when present', async function () {" to "it('returns the API-provided
handle when present', function () {"). Ensure you update all four it(...) blocks
referenced in the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 214d4eb7-61fa-4d48-86a7-899a6980185e
📒 Files selected for processing (16)
apps/activitypub/src/components/feed/feed-item.tsxapps/activitypub/src/components/global/ap-avatar.tsxapps/activitypub/src/components/global/ap-reply-box.tsxapps/activitypub/src/components/global/profile-preview-hover-card.tsxapps/activitypub/src/components/modals/new-note-modal.tsxapps/activitypub/src/utils/get-handle.tsapps/activitypub/src/utils/handle-profile-click.tsapps/activitypub/src/utils/posts.tsapps/activitypub/src/views/feed/note.tsxapps/activitypub/src/views/inbox/components/reader.tsxapps/activitypub/src/views/profile/components/actor-list.tsxapps/activitypub/test/unit/utils/get-handle.test.tsxapps/activitypub/test/unit/utils/get-username.test.tsxapps/activitypub/test/unit/utils/posts.test.tsghost/core/content/themes/casperghost/core/content/themes/source
💤 Files with no reviewable changes (1)
- apps/activitypub/test/unit/utils/get-username.test.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/activitypub/test/unit/utils/get-handle.test.tsx (1)
4-4: ⚡ Quick winRemove unnecessary
asynckeywords from test functions.All four test functions are declared as
asyncbut none useawaitor perform asynchronous operations. ThegetHandleutility appears to be synchronous. Removing theasynckeywords improves clarity and avoids misleading future maintainers.♻️ Proposed fix
- it('returns the API-provided handle when present', async function () { + it('returns the API-provided handle when present', function () { const user = { handle: '`@index`@activitypub.example', preferredUsername: 'index', id: 'https://www.platformer.news/' }; const result = getHandle(user); expect(result).toBe('`@index`@activitypub.example'); }); - it('returns the formatted handle', async function () { + it('returns the formatted handle', function () { const user = { preferredUsername: 'index', id: 'https://www.platformer.news/' }; const result = getHandle(user); expect(result).toBe('`@index`@platformer.news'); }); - it('returns a default handle if the user object is missing data', async function () { + it('returns a default handle if the user object is missing data', function () { const user = { preferredUsername: '', id: '' }; const result = getHandle(user); expect(result).toBe('`@unknown`@unknown'); }); - it('returns a default handle if url parsing fails', async function () { + it('returns a default handle if url parsing fails', function () { const user = { preferredUsername: 'index', id: 'not-a-url' }; const result = getHandle(user); expect(result).toBe('`@unknown`@unknown'); });Also applies to: 16-16, 27-27, 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/activitypub/test/unit/utils/get-handle.test.tsx` at line 4, Remove the unnecessary async keywords from the four test declarations in apps/activitypub/test/unit/utils/get-handle.test.tsx (the it blocks named "returns the API-provided handle when present" and the other three tests) because getHandle is synchronous and none of these tests use await; update each it(...) declaration to be a plain synchronous function (remove async from the function signature) so the tests accurately reflect their synchronous behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/activitypub/test/unit/utils/get-handle.test.tsx`:
- Line 4: Remove the unnecessary async keywords from the four test declarations
in apps/activitypub/test/unit/utils/get-handle.test.tsx (the it blocks named
"returns the API-provided handle when present" and the other three tests)
because getHandle is synchronous and none of these tests use await; update each
it(...) declaration to be a plain synchronous function (remove async from the
function signature) so the tests accurately reflect their synchronous behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e76d2848-db1a-4fe2-b2a0-d0fd1cd463d4
📒 Files selected for processing (15)
apps/activitypub/package.jsonapps/activitypub/src/components/feed/feed-item.tsxapps/activitypub/src/components/global/ap-avatar.tsxapps/activitypub/src/components/global/ap-reply-box.tsxapps/activitypub/src/components/global/profile-preview-hover-card.tsxapps/activitypub/src/components/modals/new-note-modal.tsxapps/activitypub/src/utils/get-handle.tsapps/activitypub/src/utils/handle-profile-click.tsapps/activitypub/src/utils/posts.tsapps/activitypub/src/views/feed/note.tsxapps/activitypub/src/views/inbox/components/reader.tsxapps/activitypub/src/views/profile/components/actor-list.tsxapps/activitypub/test/unit/utils/get-handle.test.tsxapps/activitypub/test/unit/utils/get-username.test.tsxapps/activitypub/test/unit/utils/posts.test.ts
💤 Files with no reviewable changes (1)
- apps/activitypub/test/unit/utils/get-username.test.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/activitypub/package.json
- apps/activitypub/src/components/modals/new-note-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/activitypub/src/utils/posts.ts
- apps/activitypub/src/components/global/ap-reply-box.tsx
- apps/activitypub/src/views/profile/components/actor-list.tsx
- apps/activitypub/src/views/feed/note.tsx
- apps/activitypub/src/utils/get-handle.ts
- apps/activitypub/src/components/global/profile-preview-hover-card.tsx
- apps/activitypub/src/views/inbox/components/reader.tsx
- apps/activitypub/src/utils/handle-profile-click.ts
- apps/activitypub/src/components/global/ap-avatar.tsx
- apps/activitypub/src/components/feed/feed-item.tsx
- apps/activitypub/test/unit/utils/posts.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref TryGhost/ActivityPub#1821
In TryGhost/ActivityPub#1821, we're introducing support for changing the displayed domain on a social web handle to a different subdomain or another domain they own. For example, a site hosted on jamie.larson.com can choose to expose their handle as @jamie@larson.com (instead of @index@jamie.larson.com).
In Notes/Reader, we were re-constructing the displayed handle from the ap_id, which meant we did not correctly displayed the new handle returned by the API.
Two additional tidy-ings:
actor.handle || getHandle(actor)togetHandle(actor)