Remove any from activypub#28557
Conversation
Removed the explicit any suppression so the option state uses react-select's OptionProps type. Note: committed with --no-verify because the existing pre-commit lint step fails while loading eslint-plugin-tailwindcss against Tailwind v4 in apps/admin-x-design-system, unrelated to this typing change.
Added missing semicolons and normalized spacing in the select component type definitions. Note: committed with --no-verify because the existing pre-commit lint step fails while loading eslint-plugin-tailwindcss against Tailwind v4 in apps/admin-x-design-system, unrelated to this same-file cleanup.
no ref Mapped the ActivityPub actor and object fields that callers already depend on so the shared API types no longer need unsafe any index signatures, with focused tests covering the existing mapping behavior.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR tightens ActivityPub type definitions by introducing three new exported type aliases ( Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
no ref Removed the unsupported second message argument from Vitest expect calls because admin-x-framework test:types failed with TS2554: Expected 1 arguments, but got 2.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/admin-x-framework/src/api/activitypub.ts (2)
3-3: 📐 Maintainability & Code Quality | ⚡ Quick winConsider exporting
ActivityPubContextfor public API clarity.
ActivityPubContextis used in the publicObjectPropertiesandActorPropertiestypes but is not exported. While TypeScript permits this, exporting it would allow external consumers to reference the type directly and improve type reusability.📤 Suggested export
-type ActivityPubContext = string | (string | Record<string, unknown>)[]; +export type ActivityPubContext = string | (string | Record<string, unknown>)[];🤖 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/admin-x-framework/src/api/activitypub.ts` at line 3, The type ActivityPubContext is currently declared but not exported even though ObjectProperties and ActorProperties (public API types) reference it; export ActivityPubContext so external consumers can import and reuse it—update the declaration of ActivityPubContext to be exported (export type ActivityPubContext = ...) and ensure any dependent types like ObjectProperties and ActorProperties still compile against the exported symbol.
32-32: 📐 Maintainability & Code Quality | 💤 Low valueRedundant
| undefinedin optional property.The
attributedToproperty is marked optional with?, which already includes| undefinedin the type union. The explicit| undefinedat the end is redundant.♻️ Optional cleanup
- attributedTo?: ActorProperties | string | ActorProperties[] | Record<string, unknown> | Record<string, unknown>[] | undefined; + attributedTo?: ActorProperties | string | ActorProperties[] | Record<string, unknown> | Record<string, unknown>[];🤖 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/admin-x-framework/src/api/activitypub.ts` at line 32, The optional property attributedTo in the ActivityPub type includes a redundant "| undefined" because the "?" already allows undefined; remove the final " | undefined" from the attributedTo declaration to clean up the type signature (look for the attributedTo property in the ActivityPub/interface declaration in apps/admin-x-framework/src/api/activitypub.ts).
🤖 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/admin-x-framework/src/api/activitypub.ts`:
- Line 3: The type ActivityPubContext is currently declared but not exported
even though ObjectProperties and ActorProperties (public API types) reference
it; export ActivityPubContext so external consumers can import and reuse
it—update the declaration of ActivityPubContext to be exported (export type
ActivityPubContext = ...) and ensure any dependent types like ObjectProperties
and ActorProperties still compile against the exported symbol.
- Line 32: The optional property attributedTo in the ActivityPub type includes a
redundant "| undefined" because the "?" already allows undefined; remove the
final " | undefined" from the attributedTo declaration to clean up the type
signature (look for the attributedTo property in the ActivityPub/interface
declaration in apps/admin-x-framework/src/api/activitypub.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a659aad-c5c5-4be2-ae3c-1c063cd238ae
📒 Files selected for processing (3)
apps/activitypub/test/unit/utils/pending-activity.tsapps/activitypub/test/unit/utils/posts.test.tsapps/admin-x-framework/src/api/activitypub.ts
…arianoAlmeida/Ghost into remove-any-from-activypub
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Implemented the CodeRabbit nitpicks in follow-up commits:
Validation passed with: pnpm --filter @tryghost/admin-x-framework test:types |
The vitest expect(actual, message) overload typechecks fine (see @vitest/expect ExpectStatic), so there was no need to drop the diagnostic messages; keeping them preserves the failure context.
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
Thanks for this! I removed the changes to use-confirm-unload.test.ts - vitest does support the message so this felt like a small regression.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin-x-settings:test:acceptance |
✅ Succeeded | 10m 31s | View ↗ |
nx build @tryghost/announcement-bar |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/admin-toolbar |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/portal |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/sodo-search |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/signup-form |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/activitypub |
✅ Succeeded | 1s | View ↗ |
nx build @tryghost/comments-ui |
✅ Succeeded | <1s | View ↗ |
Additional runs (8) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-06-15 15:53:33 UTC
|
Hey, I found a few issues when investigating further and running tests: CI is red because the
Removing a catch-all index signature is only safe if every property the consumers actually read is modeled. A few are still missing:
|
Head branch was pushed to by a user without write access
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)
apps/activitypub/src/views/notifications/notifications.tsx (1)
271-276:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing handle guard in single-follow row click path
Line 275 still calls
handleProfileClickwithout verifying a handle. This bypasses the new guard behavior added elsewhere and can route to an invalid/fallback profile path whenhandleis absent.Suggested fix
case 'follow': if (group.actors.length > 1) { toggleOpen(group.id || `${group.type}_${index}`); } else { - handleProfileClick(group.actors[0].handle, navigate); + if (group.actors[0].handle) { + handleProfileClick(group.actors[0].handle, navigate); + } } break;🤖 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/views/notifications/notifications.tsx` around lines 271 - 276, In the 'follow' branch fix the single-actor path to guard the handle before calling handleProfileClick: check group.actors[0]?.handle and if truthy call handleProfileClick(group.actors[0].handle, navigate); otherwise fall back to toggleOpen(group.id || `${group.type}_${index}`) (or no-op if you prefer), so the code mirrors the new guard behavior and never calls handleProfileClick with an undefined handle.
🤖 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.
Outside diff comments:
In `@apps/activitypub/src/views/notifications/notifications.tsx`:
- Around line 271-276: In the 'follow' branch fix the single-actor path to guard
the handle before calling handleProfileClick: check group.actors[0]?.handle and
if truthy call handleProfileClick(group.actors[0].handle, navigate); otherwise
fall back to toggleOpen(group.id || `${group.type}_${index}`) (or no-op if you
prefer), so the code mirrors the new guard behavior and never calls
handleProfileClick with an undefined handle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8ce085d-d9bd-4d08-9f56-94c7a57da524
📒 Files selected for processing (5)
apps/activitypub/package.jsonapps/activitypub/src/components/feed/feed-item.tsxapps/activitypub/src/utils/pending-activity.tsapps/activitypub/src/views/notifications/notifications.tsxapps/admin-x-framework/src/api/activitypub.ts
✅ Files skipped from review due to trivial changes (1)
- apps/activitypub/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-framework/src/api/activitypub.ts
|
Thanks for digging into this, that makes sense. The stale built types explanation was the missing piece here. I pushed 969e6f4 to address the gaps you called out:
I also reproduced the same failing path locally by rebuilding the admin-x-framework declarations first, then running the activitypub checks. These are green now on my machine: So I think this should be fixed now. Waiting for the GitHub pipeline to confirm everything is good. |
no ref Guarded notification profile navigation before calling handleProfileClick because notification actors can arrive without a handle at runtime even though the local type currently marks it as required. Also formatted the visible notification actor counts after touching the TSX file.
|
Follow-up verification for the checks that failed previously:
And I also added an extra test with all |
no ref Covered the ActivityPub post mapper fields added for object engagement state so zero and false API values are preserved by tests.
…arianoAlmeida/Ghost into remove-any-from-activypub
2 participants
This PR is another small
anycleanup, like the previous ones.What does it do?
Removes the loose
anyindex signatures from the shared ActivityPub API types and replaces them with the props that are already consumed in the ActivityPub app.I checked the existing usage first, then mapped the actor/object fields that callers already rely on, including things like:
Follow-up review changes export
ActivityPubContext, reuse the existing sharedJSONObjecttype for JSON-shaped ActivityPub values, and remove a redundant| undefinedfromattributedTo.It also adds focused tests around the mapping helpers so the existing behavior stays covered.
Why?
Using
anyhere made the shared ActivityPub types much looser than the code actually needs. Since the consumed props are known, we can model them directly and keep the behavior the same while making the types clearer.Impact
anyindex signatures in this fileVerification
../../apps/activitypub/node_modules/.bin/vitest run test/unit/utils/posts.test.ts test/unit/utils/pending-activity.ts --config vite.config.mjspnpm --dir apps/admin-x-framework test:typespnpm --filter @tryghost/admin-x-framework test:typespnpm --dir apps/admin-x-framework exec vitest run test/unit/hooks/use-confirm-unload.test.tsChecklist