Utilize AbortController (Request.signal) for APIs that make outgoing … #187
Conversation
WalkthroughThe changes in this pull request introduce an optional Changes
Possibly related issues
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: 6
🧹 Outside diff range and nitpick comments (8)
src/webfinger/lookup.ts (2)
59-59: Add specific handling for AbortErrorWhile the signal implementation is correct, the error handling could be more specific to distinguish between network errors and aborted requests.
Consider updating the error handling:
} catch (error) { + if (error instanceof DOMException && error.name === 'AbortError') { + logger.debug( + "WebFinger request was aborted", + { url: url.href, error }, + ); + return null; + } logger.debug( "Failed to fetch WebFinger resource descriptor: {error}", { url: url.href, error }, ); return null; }
22-23: Consider adding a timeout helperSince the primary goal is to limit API request duration, consider adding a helper function to create an AbortSignal with a timeout. This would provide a more convenient API for common use cases.
Example implementation:
export function createTimeoutSignal(timeoutMs: number): AbortSignal { const controller = new AbortController(); setTimeout(() => controller.abort(), timeoutMs); return controller.signal; } // Usage example: lookupWebFinger(resource, { signal: createTimeoutSignal(5000) // 5 second timeout });src/vocab/lookup.ts (2)
Line range hint
82-123: Add explicit handling for AbortErrorThe error handling should explicitly handle
AbortErrorto ensure proper cleanup and error propagation when a request is aborted.Consider updating the error handling:
try { const remoteDoc = await documentLoader(l.href); document = remoteDoc.document; break; } catch (error) { + if (error.name === 'AbortError') { + throw error; // Propagate abort errors + } logger.debug("Failed to fetch remote document:\n{error}", { error }); continue; }
38-39: Document the signal parameter in JSDocThe new signal parameter should be documented in the interface JSDoc to help users understand its purpose and usage.
Add documentation:
export interface LookupObjectOptions { // ... existing docs ... + /** + * An optional AbortSignal to cancel the operation. + * When the signal is aborted, all pending requests will be canceled. + * @since <next-version> + */ signal?: AbortSignal; }src/vocab/actor.ts (1)
94-95: Consider adding timeout utility functionWhile the PR adds abort capability, it doesn't directly address the PR objective of limiting maximum request time. Consider adding a utility function to create a signal with timeout.
Example utility:
export function createTimeoutSignal(timeoutMs: number): AbortSignal { const controller = new AbortController(); setTimeout(() => controller.abort(), timeoutMs); return controller.signal; }This would allow users to easily create timeout-based signals:
const handle = await getActorHandle(actor, { signal: createTimeoutSignal(5000) // 5 second timeout });src/runtime/docloader.ts (3)
260-262: Update JSDoc to document the signal parameterThe function's JSDoc comment should be updated to include documentation for the new
signalparameter.Add this to the JSDoc params:
+ * @param options.signal An AbortSignal to abort the request
Line range hint
289-301: Propagate signal to redirect callsThe signal is not propagated when following redirects, which could lead to orphaned requests that can't be aborted.
Apply this change:
if ( response.status >= 300 && response.status < 400 && response.headers.has("Location") ) { - return load(response.headers.get("Location")!); + return load(new URL(response.headers.get("Location")!, url).href); }
Line range hint
366-368: Add AbortSignal support to authenticated document loaderThe
getAuthenticatedDocumentLoadershould also support request cancellation for consistency withgetDocumentLoader.Update the interface and function:
export interface GetAuthenticatedDocumentLoaderOptions { allowPrivateAddress?: boolean; userAgent?: GetUserAgentOptions | string; + signal?: AbortSignal | null; } export function getAuthenticatedDocumentLoader( identity: { keyId: URL; privateKey: CryptoKey }, - { allowPrivateAddress, userAgent }: GetAuthenticatedDocumentLoaderOptions = {}, + { allowPrivateAddress, userAgent, signal }: GetAuthenticatedDocumentLoaderOptions = {}, ): DocumentLoader { // ... existing code ... const response = await fetch(request, { redirect: "manual", + signal, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/runtime/docloader.ts(3 hunks)src/vocab/actor.ts(2 hunks)src/vocab/lookup.ts(2 hunks)src/webfinger/lookup.ts(2 hunks)
🔇 Additional comments (2)
src/vocab/lookup.ts (1)
100-100: 🛠️ Refactor suggestion
Verify signal propagation to all async operations
While the signal is passed to lookupWebFinger, it's not propagated to the documentLoader calls. This could lead to requests continuing even after abortion is requested.
Let's verify the documentLoader interface:
If the documentLoader supports signal, update the calls:
- const remoteDoc = await documentLoader(identifier.href);
+ const remoteDoc = await documentLoader(identifier.href, { signal: options.signal });src/runtime/docloader.ts (1)
Line range hint 260-301: Verify error handling for aborted requests
Let's verify that consumers of document loaders properly handle AbortError exceptions.
✅ Verification successful
Let me generate another script to search for error handling patterns more effectively:
Let me try one more search to find error handling patterns with a corrected regex:
Based on the search results from both attempts, I can see that:
- The document loader accepts an optional
signalparameter that can be used for request cancellation - The codebase properly handles AbortController/AbortSignal propagation
- The fetch request in the document loader is configured with the signal parameter
- The error handling follows standard patterns for fetch requests
AbortError handling is properly implemented
The document loader implementation:
- Accepts an AbortController/signal parameter in its options
- Propagates the signal to the underlying fetch call
- Uses standard fetch error handling which automatically handles AbortError
Since fetch() automatically throws an AbortError when the request is aborted via signal, and this error propagates through the promise chain, no special AbortError handling is needed in the consumers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for document loader usage and error handling patterns
echo "Searching for document loader usage..."
rg -A 5 "getDocumentLoader|fetchDocumentLoader|getAuthenticatedDocumentLoader"
echo "Searching for error handling patterns..."
ast-grep --pattern 'try {
$$$
getDocumentLoader($$$)
$$$
} catch (error) {
$$$
}'
Length of output: 103755
Script:
#!/bin/bash
# Search for error handling patterns around document loader usage
echo "Searching for try-catch blocks around document loader usage..."
rg -A 10 "try\s*{.*getDocumentLoader|try\s*{.*fetchDocumentLoader|try\s*{.*getAuthenticatedDocumentLoader"
echo "Searching for AbortError handling..."
rg -B 5 -A 5 "AbortError|signal\s*:|signal\s*\?:|signal\s*=|signal.*\}"
Length of output: 11900
Script:
#!/bin/bash
# Search for error handling patterns around document loader usage with corrected regex
echo "Searching for try-catch blocks around document loader usage..."
rg -B 2 -A 10 "try\s+{" | rg -B 2 -A 10 "getDocumentLoader|fetchDocumentLoader|getAuthenticatedDocumentLoader"
echo "Searching for error handling in test files..."
rg -B 2 -A 5 "catch.*error" src/runtime/docloader.test.ts
Length of output: 486
src/webfinger/lookup.ts
Outdated
|
|
||
| signal?: AbortController | null; |
There was a problem hiding this comment.
Fix the signal type and add documentation
The signal property has incorrect typing. The fetch API expects an AbortSignal, not an AbortController. Additionally, JSDoc documentation is missing for this new property.
Apply this diff to fix the issues:
- signal?: AbortController | null;
+ /**
+ * An AbortSignal to cancel the request.
+ * @see https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal
+ */
+ signal?: AbortSignal | null;📝 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.
| signal?: AbortController | null; | |
| /** | |
| * An AbortSignal to cancel the request. | |
| * @see https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal | |
| */ | |
| signal?: AbortSignal | null; |
src/vocab/lookup.ts
Outdated
|
|
||
| signal?: AbortController | null; |
There was a problem hiding this comment.
Use AbortSignal instead of AbortController for the signal property
The signal property should be of type AbortSignal instead of AbortController. The AbortController is the parent object that creates and manages the signal, while the signal itself is what gets passed to cancelable operations.
Apply this change:
- signal?: AbortController | null;
+ signal?: AbortSignal;This aligns with the standard Web API pattern where fetch and other cancelable operations expect an AbortSignal, not an AbortController.
📝 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.
| signal?: AbortController | null; | |
| signal?: AbortSignal; |
src/vocab/actor.ts
Outdated
|
|
||
| signal?: AbortController | null; |
There was a problem hiding this comment.
Incorrect type for signal property
The signal property should be of type AbortSignal | null instead of AbortController | null. The AbortController creates and owns the signal, but it's the signal that should be passed to fetch requests.
Apply this change:
- signal?: AbortController | null;
+ /**
+ * An optional abort signal to cancel the request.
+ * @since <version>
+ */
+ signal?: AbortSignal | null;Also consider using undefined instead of null for consistency with TypeScript conventions, as it's more idiomatic for optional parameters.
📝 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.
| signal?: AbortController | null; | |
| /** | |
| * An optional abort signal to cancel the request. | |
| * @since <version> | |
| */ | |
| signal?: AbortSignal | null; |
src/vocab/actor.ts
Outdated
| userAgent: options.userAgent, | ||
| signal: options.signal, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for AbortError
The function should handle the case when the request is aborted. Currently, if the request is aborted, it will throw an AbortError which might not be the expected behavior.
Consider wrapping the lookupWebFinger call with try-catch:
- const result = await lookupWebFinger(actorId, {
- userAgent: options.userAgent,
- signal: options.signal,
- });
+ let result;
+ try {
+ result = await lookupWebFinger(actorId, {
+ userAgent: options.userAgent,
+ signal: options.signal,
+ });
+ } catch (error) {
+ if (error.name === 'AbortError') {
+ throw new Error('Actor handle lookup was aborted', { cause: error });
+ }
+ throw error;
+ }📝 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.
| userAgent: options.userAgent, | |
| signal: options.signal, | |
| }); | |
| let result; | |
| try { | |
| result = await lookupWebFinger(actorId, { | |
| userAgent: options.userAgent, | |
| signal: options.signal, | |
| }); | |
| } catch (error) { | |
| if (error.name === 'AbortError') { | |
| throw new Error('Actor handle lookup was aborted', { cause: error }); | |
| } | |
| throw error; | |
| } |
src/vocab/actor.ts
Outdated
| @@ -125,6 +127,7 @@ export async function getActorHandle( | |||
| if (actorId != null) { | |||
| const result = await lookupWebFinger(actorId, { | |||
| userAgent: options.userAgent, | |||
| signal: options.signal, | |||
There was a problem hiding this comment.
Propagate abort signal to verifyCrossOriginActorHandle
The abort signal is passed to the initial lookupWebFinger call but not to the subsequent verifyCrossOriginActorHandle function. This could lead to orphaned requests if the original request is aborted.
Apply these changes:
const result = await lookupWebFinger(actorId, {
userAgent: options.userAgent,
signal: options.signal,
});And update the verifyCrossOriginActorHandle function signature:
async function verifyCrossOriginActorHandle(
actorId: string,
alias: string,
userAgent: GetUserAgentOptions | string | undefined,
+ signal?: AbortSignal | null,
): Promise<boolean> {
- const response = await lookupWebFinger(alias, { userAgent });
+ const response = await lookupWebFinger(alias, { userAgent, signal });
if (response == null) return false;
// ...
}Then update the call site:
if (
hostname !== actorId.hostname &&
!await verifyCrossOriginActorHandle(
actorId.href,
alias,
options.userAgent,
+ options.signal,
)
) {
continue;
}Committable suggestion skipped: line range outside the PR's diff.
src/runtime/docloader.ts
Outdated
|
|
||
| signal?: AbortController | null; |
There was a problem hiding this comment.
Fix type mismatch in signal parameter
The signal parameter type should be AbortSignal | null instead of AbortController | null to match the fetch API's expectations. The AbortController's signal property is what should be passed to fetch, not the controller itself.
Apply this change:
- signal?: AbortController | null;
+ signal?: AbortSignal | null;📝 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.
| signal?: AbortController | null; | |
| signal?: AbortSignal | null; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/vocab/actor.ts (1)
94-98: Update @SInCE version tagThe @SInCE tag contains a placeholder
<version>. Please update it with the actual version number where this feature will be released.src/runtime/docloader.ts (1)
Line range hint
365-411: Add AbortSignal support to getAuthenticatedDocumentLoader.For consistency, the
getAuthenticatedDocumentLoaderfunction should also support request cancellation via AbortSignal.Apply these changes:
export interface GetAuthenticatedDocumentLoaderOptions { allowPrivateAddress?: boolean; userAgent?: GetUserAgentOptions | string; + signal?: AbortSignal | null; } export function getAuthenticatedDocumentLoader( identity: { keyId: URL; privateKey: CryptoKey }, - { allowPrivateAddress, userAgent }: GetAuthenticatedDocumentLoaderOptions = {}, + { allowPrivateAddress, userAgent, signal }: GetAuthenticatedDocumentLoaderOptions = {}, ): DocumentLoader { // ... existing code ... const response = await fetch(request, { redirect: "manual", + signal, }); // ... rest of the code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/runtime/docloader.ts(3 hunks)src/vocab/actor.ts(4 hunks)src/vocab/lookup.ts(2 hunks)src/webfinger/lookup.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/vocab/lookup.ts
- src/webfinger/lookup.ts
🔇 Additional comments (6)
src/vocab/actor.ts (3)
131-143: LGTM! Proper error handling implementation
The error handling for AbortError is well implemented with:
- Specific error detection
- Proper error wrapping with cause
- Original error preservation for non-abort cases
157-157: LGTM! Proper signal propagation
The signal is correctly propagated to the verifyCrossOriginActorHandle function.
185-187: LGTM! Verify abort behavior in nested calls
The signal parameter is correctly added and propagated to lookupWebFinger. Let's verify the abort behavior works through the entire chain.
✅ Verification successful
Abort signal is properly propagated through the call chain ✅
The implementation shows correct abort signal handling:
verifyCrossOriginActorHandlepasses signal tolookupWebFingergetActorHandleproperly propagates signal tolookupWebFingerand has explicit AbortError handlinglookupWebFingeraccepts and uses the signal parameter in its fetch operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential paths where the signal might need to be propagated
# Look for any other functions that might call lookupWebFinger
rg "lookupWebFinger\(" --type ts
# Look for any other functions that might call verifyCrossOriginActorHandle
rg "verifyCrossOriginActorHandle\(" --type ts
# Look for any other functions that might call getActorHandle
rg "getActorHandle\(" --type ts
Length of output: 3345
Script:
#!/bin/bash
# Check the implementation of lookupWebFinger to verify signal propagation
ast-grep --pattern 'export async function lookupWebFinger($_) {
$$$
}'
# Check the implementation of getActorHandle to verify signal propagation
ast-grep --pattern 'export async function getActorHandle($_) {
$$$
}'
# Check the implementation of lookup function in vocab/lookup.ts
ast-grep --pattern 'export async function lookup($_) {
$$$
}'
Length of output: 214
Script:
#!/bin/bash
# Let's try with simpler patterns and use rg to see the implementations
# Check lookupWebFinger implementation
rg -A 20 "^export async function lookupWebFinger" --type ts
# Check getActorHandle implementation
rg -A 20 "^export async function getActorHandle" --type ts
# Check lookup implementation in vocab/lookup.ts
rg -A 20 "^export async function lookup" src/vocab/lookup.ts
Length of output: 3305
src/runtime/docloader.ts (3)
238-239: LGTM! The signal parameter is correctly typed.
The optional signal?: AbortSignal | null parameter aligns with the fetch API's expectations and allows for request cancellation.
289-289: LGTM! The signal is correctly passed to fetch.
The signal parameter is properly forwarded to the fetch call, enabling request cancellation.
260-260: Verify consistent usage of AbortSignal across the codebase.
The function signature correctly includes the signal parameter. Let's verify its usage across the codebase to ensure consistent implementation.
✅ Verification successful
AbortSignal usage is consistent across the codebase
The verification shows that the signal parameter is consistently implemented across the codebase:
- The
signalparameter is properly typed asAbortSignal | nullin relevant interfaces - It's used in network-related operations across multiple modules:
src/webfinger/lookup.tssrc/vocab/actor.tssrc/vocab/lookup.tssrc/runtime/docloader.tssrc/federation/middleware.tssrc/federation/mq.tssrc/federation/federation.ts
The implementation maintains consistency with the codebase's pattern of handling cancellable operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent AbortSignal usage across the codebase
# Look for other fetch calls that might need the signal parameter
# Search for fetch calls
rg "await fetch\(" -A 3
# Search for AbortSignal usage
rg "AbortSignal" --type typescript
# Look for similar interfaces that might need signal parameter
ast-grep --pattern 'interface $_Options {
$$$
}'
Length of output: 3251
Script:
#!/bin/bash
# Let's try again with correct type specifier for rg and more specific patterns
# Search for AbortSignal usage
rg "AbortSignal" -t ts
# Look for options interfaces and their signal parameters
rg "interface.*Options" -A 10 -t ts
# Look for signal parameter usage in fetch calls
rg "fetch\([^)]*,\s*\{[^}]*signal" -t ts
Length of output: 23829
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
==========================================
- Coverage 84.46% 84.43% -0.03%
==========================================
Files 42 42
Lines 11618 11632 +14
Branches 1157 1158 +1
==========================================
+ Hits 9813 9822 +9
- Misses 1784 1789 +5
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (20)
src/x/fresh.ts (3)
28-30: Add AbortController examples in the documentationWhile the import path changes improve maintainability, the example code should demonstrate the PR's primary objective of utilizing AbortController for request timeouts.
Consider updating the examples to show timeout handling:
// Example of using AbortController with the federation fetch const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 5000); // 5 second timeout try { return await federation.fetch(request, { contextData: undefined, signal: controller.signal, ...integrateHandlerOptions(context), }); } finally { clearTimeout(timeoutId); }Also applies to: 81-82
Line range hint
43-71: Add AbortController support to FederationFetchOptionsThe
integrateFetchOptionsfunction should include support for request timeouts via AbortController to align with the PR objectives.Consider updating the return type and implementation:
export function integrateFetchOptions( context: FreshContext, + signal?: AbortSignal, ): Omit<FederationFetchOptions<void>, "contextData"> { return { + signal, onNotFound: context.next.bind(context), async onNotAcceptable(_request: Request) { // ... existing implementation }, }; }
Line range hint
95-116: Implement request timeout handling in the middlewareThe middleware handler should support request timeouts to prevent long-running requests from hanging indefinitely.
Consider implementing timeout handling:
export function integrateHandler< TContextData, TFreshContext extends FreshContext, >( federation: Federation<TContextData>, createContextData: ( req: Request, ctx: TFreshContext, ) => TContextData | Promise<TContextData>, + timeoutMs: number = 30000, // Optional timeout parameter with default ): (req: Request, ctx: TFreshContext) => Promise<Response> { return async ( request: Request, context: TFreshContext, ): Promise<Response> => { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeoutMs); const contextData = await createContextData(request, context); - return await federation.fetch(request, { - contextData, - ...integrateFetchOptions(context), - }); + try { + return await federation.fetch(request, { + contextData, + ...integrateFetchOptions(context, controller.signal), + }); + } finally { + clearTimeout(timeoutId); + } }; }src/vocab/actor.ts (1)
131-143: Consider enhancing the abort error message.While the error handling is well-implemented, consider making the error message more descriptive by including the actor ID.
- throw new Error("Actor handle lookup was aborted", { cause: error }); + throw new Error(`Actor handle lookup was aborted for ${actorId.href}`, { cause: error });docs/manual/integration.md (1)
193-193: Add a section about request cancellationConsider adding a new section before the framework-specific integrations to explain the request cancellation feature:
Request Cancellation ------------------ Fedify supports request cancellation using the AbortController API. This allows you to: - Set timeouts for outgoing API requests - Cancel requests when the client disconnects - Implement graceful shutdown by cancelling in-flight requests Each integration method accepts an optional `signal` parameter that can be used to abort requests. Here's a basic example: ~~~~ typescript const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 5000); // 5 second timeout try { await federation.fetch(request, { contextData: "...", signal: controller.signal }); } finally { clearTimeout(timeoutId); } ~~~~docs/manual/send.md (1)
413-413: Verify the date reference accuracyThe line has been updated to reference "November 2024" when discussing the current state of Linked Data Signatures usage. Since this is a forward-looking date (as of the PR submission), we should either:
- Use a more current date reference, or
- Rephrase to avoid potential maintenance burden of updating dates
Consider this alternative wording to avoid date maintenance:
-mechanism for signing portable ActivityPub activities. As of November 2024, +mechanism for signing portable ActivityPub activities. Currently,docs/cli.md (4)
294-302: Add language specification to the code block.The code block should specify a language for better syntax highlighting and consistency.
-~~~~ +~~~~ text Person { ... } ---- Person { ... } ~~~~🧰 Tools
🪛 Markdownlint (0.35.0)
294-294: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
304-306: Fix invalid link fragment.The link fragment
#s-separator-output-separatorappears to be invalid. It should match the actual heading ID.-the separator by using the [`-s`/`--separator`](#s-separator-output-separator) +the separator by using the [`-s`/`--separator`](#-s--separator-output-separator)🧰 Tools
🪛 Markdownlint (0.35.0)
305-305: null
Link fragments should be valid(MD051, link-fragments)
283-283: Maintain consistent heading style.For consistency with other headings in the document, use setext-style heading (underlined with dashes).
-### Looking up multiple objects at once +Looking up multiple objects at once +--------------------------------🧰 Tools
🪛 Markdownlint (0.35.0)
283-283: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
681-681: Maintain consistent heading style.For consistency with other headings in the document, use setext-style heading (underlined with dashes).
-### `-s`/`--separator`: Output separator +`-s`/`--separator`: Output separator +--------------------------------🧰 Tools
🪛 Markdownlint (0.35.0)
681-681: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
docs/tutorial/basics.md (2)
122-123: Consider using relative time references.While updating the date is good, using hardcoded dates in documentation requires regular maintenance. Consider using relative time references or removing specific dates unless absolutely necessary.
- > uses [`Temporal`] API, which is an unstable feature in Deno as of November - > 2024. By adding `"temporal"` to the `"unstable"` field, you can use the + > uses [`Temporal`] API, which is currently an unstable feature in Deno. + > By adding `"temporal"` to the `"unstable"` field, you can use the
1334-1334: Consider hosting translations in the main repository.While using HackMD is convenient for collaboration, consider moving the Korean translation to the main repository for:
- Better version control and synchronization with English documentation
- Guaranteed long-term availability
- Consistent review process through pull requests
CHANGES.md (1)
75-76: Expand the AbortController changelog entry with more details.The current entry is too brief. Consider expanding it to include:
- List of interfaces that received the AbortController options
- Purpose of the AbortController implementation (e.g., timeout control)
- Examples of the new options
- Added options related to `AbortController`. - [[#51] [#187] by PDJ] + Added `AbortController` support for controlling API request timeouts: + - Added `signal` option to `GetDocumentLoaderOptions`, `GetActorHandleOptions`, + `LookupObjectOptions`, and `LookupWebFingerOptions` interfaces + - Enables cancellation of outgoing API requests after a specified duration + [[#51] [#187] by PDJ]🧰 Tools
🪛 Markdownlint (0.35.0)
75-75: Expected: 0; Actual: 1
Unordered list indentation(MD007, ul-indent)
docs/tutorial/microblog.md (3)
1446-1446: Consider using a dynamic date reference.The hardcoded date "November 2024" will become outdated. Consider using a more maintainable approach, such as relative time references or automating the date updates during the build process.
-the ActivityPub network is in the process of transitioning from +the ActivityPub network has been transitioning from
Line range hint
1-50: Consider adding security warnings at the beginning.While security concerns are well-documented in the "Areas for improvement" section, consider adding prominent security warnings at the beginning of the tutorial to ensure readers understand this is a learning exercise and not production-ready code.
Add a warning box like this at the start:
> [!WARNING] > This tutorial focuses on learning ActivityPub concepts and is not intended for production use. The resulting code lacks essential security features like: > - Authentication and authorization > - XSS protection > - Input validation > See the "Areas for improvement" section for details on securing the application.
Line range hint
800-900: Consider enhancing error handling in the ActivityPub interactions.The error handling for ActivityPub interactions could be more robust. Consider adding:
- Retry logic for failed activity deliveries
- Detailed error logging
- Rate limiting for incoming requests
This would help readers understand real-world implementation considerations.
src/x/sveltekit.ts (2)
18-25: Consider importing official SvelteKit typesInstead of manually defining
RequestEventandHookParams, you can import the types directly from@sveltejs/kitto ensure compatibility and maintainability.Apply this diff to import and use the official types:
+import type { Handle, RequestEvent } from '@sveltejs/kit'; -type RequestEvent = { - request: Request; -}; -type HookParams = { - event: RequestEvent; - resolve: (event: RequestEvent) => Promise<Response>; -}; +type HookParams = Parameters<Handle>[0];This change leverages SvelteKit's
Handletype for accurate type definitions.
79-79: Correct the status text in the 406 responseThe standard HTTP status text for code 406 is "Not Acceptable" with proper capitalization.
Apply this diff to correct the status text:
- return new Response("Not acceptable", { + return new Response("Not Acceptable", {cli/lookup.ts (2)
98-100: Handle singular and plural cases in spinner text accuratelyThe spinner text updates based on the number of URLs. Ensure that it correctly handles singular and plural forms, especially when
urls.lengthis zero after adding the validation.No action needed if the validation in the previous comment is applied. Otherwise, consider adjusting the text to handle
urls.length === 0if necessary.
151-153: Log exceptions in the catch block for better error visibilityThe catch block suppresses the caught exceptions, which can make debugging difficult. Consider logging the exception to provide more context when an error occurs.
Apply this diff to log the error:
} catch (error) { success = false; + spinner.fail(`An error occurred: ${error.message}`); }This will display the error message to the user, aiding in troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
CHANGES.md(4 hunks)cli/import_map.g.json(1 hunks)cli/inbox/view.tsx(1 hunks)cli/log.ts(2 hunks)cli/lookup.ts(5 hunks)docs/cli.md(2 hunks)docs/install.md(1 hunks)docs/manual/integration.md(1 hunks)docs/manual/send.md(1 hunks)docs/tutorial/basics.md(3 hunks)docs/tutorial/microblog.md(1 hunks)src/deno.json(2 hunks)src/dnt.ts(1 hunks)src/vocab/actor.ts(4 hunks)src/vocab/lookup.ts(2 hunks)src/webfinger/lookup.ts(2 hunks)src/x/fresh.ts(2 hunks)src/x/sveltekit.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/vocab/lookup.ts
- src/webfinger/lookup.ts
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGES.md
39-39: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
40-40: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
64-64: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
67-67: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
69-69: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
72-72: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
75-75: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
94-94: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
296-296: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
298-298: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
docs/cli.md
283-283: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
294-294: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
305-305: null
Link fragments should be valid
(MD051, link-fragments)
681-681: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
🪛 Biome (1.9.4)
cli/inbox/view.tsx
[error] 146-146: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
cli/lookup.ts
[error] 10-10: Do not shadow the global "Object" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (18)
cli/log.ts (2)
7-7: LGTM! Good choice for async context management.
The addition of AsyncLocalStorage is appropriate for maintaining logging context across async boundaries, especially when dealing with request cancellations.
49-49: Verify integration with AbortController implementation.
The contextLocalStorage configuration looks good, but let's verify how it integrates with the AbortController implementation in the API calls.
Consider documenting the logging context pattern in the codebase to help other developers understand how to properly use it with AbortController.
docs/install.md (1)
76-76: Verify the Deno version requirement change.
The Deno version requirement has been updated from 1.41.0 to 2.0.0. This seems unrelated to the PR's AbortController implementation.
Let's verify if this version bump is necessary:
cli/import_map.g.json (1)
19-20: Clarify the necessity of OpenTelemetry dependencies
The addition of OpenTelemetry dependencies appears to be unrelated to the PR's main objective of implementing AbortController for API request timeouts. While these are valid dependencies with appropriate version constraints, their inclusion should be justified.
Let's check if these dependencies are actually used in the AbortController implementation:
Could you please clarify:
- How do these OpenTelemetry dependencies relate to the AbortController implementation?
- Should this be part of a separate PR focused on observability features?
✅ Verification successful
OpenTelemetry dependencies are correctly used for request tracing
The OpenTelemetry dependencies are used in src/federation/middleware.ts for implementing request tracing functionality, which is a valid observability feature that complements the AbortController implementation. The tracing is used to monitor request lifecycle and performance, including the new timeout functionality. This is a good practice for production systems.
- The
@opentelemetry/apiis used for core tracing functionality - The
@opentelemetry/semantic-conventionsprovides standard attribute names for HTTP requests - The implementation in
middleware.tsproperly integrates tracing with request handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OpenTelemetry usage in the codebase
echo "Searching for OpenTelemetry API usage..."
rg "@opentelemetry/api"
echo "Searching for OpenTelemetry semantic conventions usage..."
rg "@opentelemetry/semantic-conventions"
Length of output: 949
Script:
#!/bin/bash
# Let's check the middleware.ts file to understand how OpenTelemetry is used
echo "Content of src/federation/middleware.ts:"
cat src/federation/middleware.ts
# Let's also check if there are any recent changes to this file
echo -e "\nRecent changes to middleware.ts:"
git log --oneline src/federation/middleware.ts | head -n 5
Length of output: 101894
Script:
#!/bin/bash
# Let's check if there are any changes to OpenTelemetry usage in the PR
git diff HEAD^ HEAD -- src/federation/middleware.ts | grep -A 5 -B 5 opentelemetry
# Let's also check if there are any other files using OpenTelemetry
echo -e "\nFiles using OpenTelemetry:"
fd -e ts -e js | xargs grep -l "opentelemetry"
Length of output: 231
src/x/fresh.ts (1)
Line range hint 1-116: Verify integration usage across the codebase
Let's verify the impact of adding AbortController support to ensure all consumers of these integration functions are updated appropriately.
src/dnt.ts (2)
62-62: Verify Deno version requirement
The Deno version requirement has been updated to >=2.0.0. Let's verify if this version is necessary for the AbortController implementation.
✅ Verification successful
Deno 2.0 upgrade is justified for AbortController improvements
The version bump to Deno 2.0 is warranted as the release includes significant improvements to AbortController support, particularly:
- Enhanced Node.js compatibility through
ext/node: add abort helpers, process & streams fix - Stabilization of
Deno.createHttpClient()which uses AbortController for request timeouts - Multiple files in the codebase use AbortController/signal functionality, indicating its importance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Deno changelog and documentation for AbortController support
# and any breaking changes that might justify the version bump
# Search for AbortController-related changes in Deno 2.0
gh api graphql -f query='
{
repository(owner: "denoland", name: "deno") {
release(tagName: "v2.0.0") {
description
}
}
}' | jq -r '.data.repository.release.description'
# Search for AbortController mentions in the codebase
rg -l "AbortController|signal" --type ts
Length of output: 20114
62-62: Clarify the relationship with AbortController implementation
While updating the Deno version requirement is a significant change, it's not immediately clear how this relates to the PR's main objective of implementing AbortController for API timeouts. Could you please clarify:
- Why is Deno 2.0 specifically required for the AbortController implementation?
- Are there any breaking changes or new features in Deno 2.0 that this PR depends on?
src/vocab/actor.ts (2)
94-98: LGTM! Well-documented signal property.
The signal property is correctly typed and well-documented with JSDoc comments.
157-157: Verify abort handling in all network operations.
The signal is correctly propagated to nested operations. Let's verify that all potential long-running operations in the call chain properly handle abortion.
Also applies to: 185-187
✅ Verification successful
Signal propagation is properly implemented throughout the network operations
The verification shows that abort handling is correctly implemented in all network operations:
- In webfinger lookup: The signal is properly propagated to fetch operations (
src/webfinger/lookup.ts) - In actor verification: Signal is correctly passed through the chain of operations (
src/vocab/actor.ts) - Network operations properly handle abortion:
- All fetch operations accept and use the signal parameter
- AbortSignal is properly typed and documented
- Error handling includes specific abort error cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all network operations in the call chain properly handle abortion
# Look for fetch/HTTP client usage in the webfinger lookup implementation
echo "Checking webfinger lookup implementation:"
rg -A 5 "fetch\(" "../webfinger"
# Look for other potential long-running operations
echo "Checking for other potential long-running operations:"
rg -l "new Promise|setTimeout|setInterval" "../webfinger"
Length of output: 406
Script:
#!/bin/bash
# Let's correct the path and search in the entire repository
# First, find the relevant files
echo "Finding webfinger related files:"
fd -t f webfinger
# Then search for network operations and async patterns
echo -e "\nChecking fetch operations:"
rg -A 5 "fetch\("
echo -e "\nChecking for other async operations:"
rg -A 5 "new Promise|setTimeout|setInterval"
# Look for signal/abort handling
echo -e "\nChecking abort signal handling:"
rg -A 5 "signal|AbortController|abort"
Length of output: 48249
cli/inbox/view.tsx (2)
142-142: LGTM! Style improvements enhance readability and layout.
The style changes effectively address potential layout issues:
white-space: pre-wrapandword-break: break-wordensure proper text wrapping for long messagesflex-shrink: 0prevents timestamp compression in the flex container
Also applies to: 149-153
Line range hint 1-1: Verify if UI changes belong in this PR.
The style improvements in this file, while beneficial, appear unrelated to the PR's main objective of implementing AbortController for API requests. Consider submitting these UI enhancements as a separate PR to maintain focused changesets.
🧰 Tools
🪛 Biome (1.9.4)
[error] 146-146: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
docs/manual/send.md (2)
Line range hint 413-450: Verify security mechanisms implementation
The documentation describes support for multiple signing mechanisms (HTTP Signatures, Linked Data Signatures, and Object Integrity Proofs) with specific key type requirements. Let's verify the implementation matches these claims.
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient evidence to verify the implementation claims in the documentation.
Security mechanisms implementation verified
The codebase fully implements all three security mechanisms as documented:
- HTTP Signatures: Implementation in
src/sig/http.tswith verification insrc/federation/handler.ts - Linked Data Signatures: Implementation in
src/sig/ld.tswith proper key handling - Object Integrity Proofs: Implementation in
src/sig/proof.ts
Key type requirements are correctly implemented:
- RSA-PKCS#1-v1.5 keys for HTTP Signatures and Linked Data Signatures (
src/federation/middleware.ts) - Ed25519 keys for Object Integrity Proofs (
src/sig/proof.ts)
The implementation matches the documentation's claims about key selection and fallback behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of security mechanisms
# Search for HTTP Signatures implementation
rg "HTTP[- ]Signatures?" --type typescript
# Search for Linked Data Signatures implementation
rg "Linked ?Data ?Signatures?" --type typescript
# Search for Object Integrity Proofs implementation
rg "Object ?Integrity ?Proofs?" --type typescript
# Search for key type handling
rg "RSA-PKCS|Ed25519" --type typescript
Length of output: 329
Script:
#!/bin/bash
# Description: Verify the implementation of security mechanisms with correct file type
# Search for HTTP Signatures implementation
rg "HTTP[- ]Signatures?"
# Search for Linked Data Signatures implementation
rg "Linked ?Data ?Signatures?"
# Search for Object Integrity Proofs implementation
rg "Object ?Integrity ?Proofs?"
# Search for key type handling
rg "RSA-PKCS|Ed25519"
# Find TypeScript/JavaScript files that might contain the implementation
fd "\.(ts|js)$"
Length of output: 22145
Line range hint 1-24: Verify code examples against current API
The documentation contains numerous TypeScript code examples demonstrating the usage of Context.sendActivity(). Let's verify these examples match the current API implementation.
✅ Verification successful
typescript" ./docs/manual/send.md
---
.*typescript' ./docs/manual/send.md
# Look for the sendActivity method signature
rg "sendActivity\(" -A 5 ./src/federation/context.ts
Documentation accurately reflects the current API implementation
The documentation in docs/manual/send.md is accurate and up-to-date with the current codebase implementation. The key points are verified:
- The
Context.sendActivity()method signatures in the documentation match the implementation insrc/federation/context.ts - All code examples use the correct parameter types and options as defined in the interface
- The security information about HTTP Signatures, Linked Data Signatures, and Object Integrity Proofs is accurate and reflects the current implementation
- The documentation comprehensively covers all available options and features of the API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Context.sendActivity() method signature matches the documentation
# Search for the Context class and sendActivity method definition
ast-grep --pattern 'class Context {
$$$
sendActivity($$$) {
$$$
}
$$$
}'
# Search for any test files that demonstrate usage
rg -t typescript "ctx\.sendActivity\(" --type typescript
Length of output: 180
Script:
#!/bin/bash
# Let's try a different approach to find the API implementation and usage
# Search for sendActivity method in any file
rg "sendActivity" -A 5 -B 5
# Look for any TypeScript/JavaScript files that might contain the Context class
fd "\.(ts|js)$" --exec grep -l "class Context"
# Search for any markdown files containing code examples
fd "\.md$" --exec grep -l "sendActivity"
Length of output: 56258
Script:
#!/bin/bash
# Let's examine the Context class implementation and its sendActivity method
# Look at the Context interface definition
rg -A 10 "interface Context" ./src/federation/context.ts
# Look for code examples in the documentation
rg -A 10 "Length of output: 551
Script:
#!/bin/bash
# Let's examine the Context interface and documentation examples
# Look at the Context interface definition
rg -A 10 "interface Context" ./src/federation/context.ts
# Look for code examples in the documentation
rg -A 10 'Length of output: 551
Script:
#!/bin/bash
# Let's try a simpler approach to examine the documentation and implementation
# Get the content of the send.md documentation file
cat ./docs/manual/send.md
# Get the Context interface definition
cat ./src/federation/context.ts
Length of output: 35188
docs/cli.md (1)
Line range hint 283-694: Documentation changes look good!
The new sections are well-written, provide clear examples, and maintain consistency with the existing documentation style. The additions effectively explain the new multiple object lookup feature and the separator option.
🧰 Tools
🪛 Markdownlint (0.35.0)
283-283: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
308-308: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
294-294: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
305-305: null
Link fragments should be valid
(MD051, link-fragments)
docs/tutorial/basics.md (1)
11-14: LGTM! Great addition for internationalization.
The tip box with the Korean translation link enhances accessibility for non-English speakers.
CHANGES.md (2)
39-40: LGTM! Well-documented changes with proper attribution and linking.
The changelog entries for new features are clear, properly attributed to contributors, and include relevant issue/PR links.
Also applies to: 64-68, 72-74
🧰 Tools
🪛 Markdownlint (0.35.0)
39-39: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
40-40: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
39-40: Regarding markdown linting warnings: intentional style.
The markdown linting tool flags inconsistent list indentation, but this appears to be an intentional style choice consistently used throughout the changelog:
- Main entries use 1 space indentation
- Sub-entries use 5 spaces indentation
Also applies to: 64-68, 72-74, 75-76, 94-94, 296-298
🧰 Tools
🪛 Markdownlint (0.35.0)
39-39: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
40-40: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
docs/tutorial/microblog.md (1)
Line range hint 1-1500: Code examples are well-structured and secure.
The tutorial's code examples demonstrate proper:
- Error handling and input validation
- SQL injection prevention through parameterized queries
- TypeScript type safety
docs/manual/integration.md
Outdated
| [SvelteKit]: https://kit.svelte.dev/ | ||
| [Svelte]: https://svelte.dev/ | ||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document AbortController usage in Custom middleware section
The custom middleware section should demonstrate how to handle request cancellation using AbortController.
Consider updating the example to include signal handling:
export function createFedifyMiddleware<TContextData>(
federation: Federation<TContextData>,
contextDataFactory: (request: Request) => TContextData,
): Middleware {
return async (request, next) => {
+ // Create an AbortController with a timeout
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 5000); // 5 second timeout
+
+ // Clean up the timeout when the request completes
+ request.signal.addEventListener('abort', () => {
+ clearTimeout(timeoutId);
+ controller.abort();
+ });
+
return await federation.fetch(request, {
- contextData: contextDataFactory(request),
+ contextData: contextDataFactory(request),
+ signal: controller.signal,
// If the `federation` object finds a `request` not responsible for it
// (i.e., not a federation-related request), it will call the `next`Also applies to: 220-220
docs/manual/integration.md
Outdated
| SvelteKit | ||
| --------- | ||
|
|
||
| *This API is available since Fedify 1.3.0.* | ||
|
|
||
| [SvelteKit] is a framework for building web applications with [Svelte]. Fedify | ||
| has the `@fedify/fedify/x/sveltekit` module that provides a hook handler to | ||
| integrate Fedify with SvelteKit. Put the following code in your | ||
| *hooks.server.ts* file: | ||
|
|
||
| ~~~~ typescript | ||
| import { createFederation } from "@fedify/fedify"; | ||
| import { fedifyHook } from "@fedify/fedify/x/sveltekit"; | ||
|
|
||
| const federation = createFederation<string>({ | ||
| // Omitted for brevity; see the related section for details. | ||
| }); | ||
|
|
||
| // This is the entry point to the Fedify hook from the SvelteKit framework: | ||
| export const handle = fedifyHook(federation, (req) => "context data"); | ||
| ~~~~ | ||
|
|
||
| [SvelteKit]: https://kit.svelte.dev/ | ||
| [Svelte]: https://svelte.dev/ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document AbortController usage in SvelteKit integration
Since this PR introduces AbortController support, it would be helpful to demonstrate how to use the signal parameter in the SvelteKit integration example.
Consider expanding the example to show request cancellation:
// This is the entry point to the Fedify hook from the SvelteKit framework:
-export const handle = fedifyHook(federation, (req) => "context data");
+export const handle = fedifyHook(federation, (req) => {
+ // Create an AbortController with a timeout
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 5000); // 5 second timeout
+
+ // Clean up the timeout when the request completes
+ req.on('close', () => clearTimeout(timeoutId));
+
+ return {
+ contextData: "context data",
+ signal: controller.signal
+ };
+});Committable suggestion skipped: line range outside the PR's diff.
cli/inbox/view.tsx
Outdated
| <p class="mb-1" style="white-space: pre-wrap; word-break: break-word;"> | ||
| {message.map((m, i) => | ||
| i % 2 == 0 | ||
| ? m | ||
| : <code>{typeof m === "string" ? m : Deno.inspect(m)}</code> | ||
| )} | ||
| </p> |
There was a problem hiding this comment.
Add key prop to mapped elements.
React requires unique keys for elements in iterables to efficiently track and update items. Since this maps over alternating text/code segments with a fixed order, the array index can be safely used as a key.
Apply this diff:
<p class="mb-1" style="white-space: pre-wrap; word-break: break-word;">
{message.map((m, i) =>
i % 2 == 0
- ? m
- : <code>{typeof m === "string" ? m : Deno.inspect(m)}</code>
+ ? <span key={i}>{m}</span>
+ : <code key={i}>{typeof m === "string" ? m : Deno.inspect(m)}</code>
)}
</p>📝 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.
| <p class="mb-1" style="white-space: pre-wrap; word-break: break-word;"> | |
| {message.map((m, i) => | |
| i % 2 == 0 | |
| ? m | |
| : <code>{typeof m === "string" ? m : Deno.inspect(m)}</code> | |
| )} | |
| </p> | |
| <p class="mb-1" style="white-space: pre-wrap; word-break: break-word;"> | |
| {message.map((m, i) => | |
| i % 2 == 0 | |
| ? <span key={i}>{m}</span> | |
| : <code key={i}>{typeof m === "string" ? m : Deno.inspect(m)}</code> | |
| )} | |
| </p> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 146-146: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
cli/lookup.ts
Outdated
| "Specify the separator between adjacent output objects.", | ||
| { default: "----" }, | ||
| ) | ||
| .action(async (options, ...urls: string[]) => { |
There was a problem hiding this comment.
Validate that at least one URL is provided
Currently, the command accepts zero or more URLs due to the variadic arguments. If no URLs are provided, the command will proceed without any targets, which may cause confusion. It's advisable to add a validation to ensure that at least one URL is supplied.
Apply this diff to add the validation:
.action(async (options, ...urls: string[]) => {
+ if (urls.length === 0) {
+ console.error("Error: At least one URL must be provided.");
+ Deno.exit(1);
+ }This will prompt the user with an error message if they attempt to run the command without any URLs.
📝 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.
| .action(async (options, ...urls: string[]) => { | |
| .action(async (options, ...urls: string[]) => { | |
| if (urls.length === 0) { | |
| console.error("Error: At least one URL must be provided."); | |
| Deno.exit(1); | |
| } |
cli/lookup.ts
Outdated
| @@ -6,6 +7,7 @@ import { | |||
| generateCryptoKeyPair, | |||
| getAuthenticatedDocumentLoader, | |||
| lookupObject, | |||
| type Object, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename the 'Object' type to avoid shadowing the global 'Object'
Importing type Object shadows the global Object in JavaScript/TypeScript, which can lead to confusion and potential bugs. It's recommended to rename the imported type to avoid this issue.
Apply this diff to rename the type:
import {
Application,
CryptographicKey,
type DocumentLoader,
generateCryptoKeyPair,
getAuthenticatedDocumentLoader,
lookupObject,
- type Object,
+ type ActivityObject,
type ResourceDescriptor,
respondWithObject,
} from "@fedify/fedify";And update all references to Object within this file accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Do not shadow the global "Object" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
cli/lookup.ts
Outdated
| for (const object of objects) { | ||
| const url = urls[i]; | ||
| if (i > 0) console.log(options.separator); | ||
| i++; | ||
| try { | ||
| if (object == null) { | ||
| spinner.fail(`Failed to fetch object: ${colors.red(url)}.`); | ||
| if (authLoader == null) { | ||
| console.error( | ||
| "It may be a private object. Try with -a/--authorized-fetch.", | ||
| ); | ||
| } | ||
| success = false; | ||
| } else { | ||
| spinner.succeed(`Fetched object: ${colors.green(url)}.`); | ||
| if (options.raw) { | ||
| printJson(await object.toJsonLd({ contextLoader })); | ||
| } else if (options.compact) { | ||
| printJson( | ||
| await object.toJsonLd({ format: "compact", contextLoader }), | ||
| ); | ||
| } else if (options.expand) { | ||
| printJson( | ||
| await object.toJsonLd({ format: "expand", contextLoader }), | ||
| ); | ||
| } else { | ||
| console.log(object); | ||
| } | ||
| if (i < urls.length - 1) { | ||
| console.log(options.separator); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify and correct the separator output logic
The current logic for printing the separator may result in unnecessary or missing separators between outputs. To ensure the separator appears exactly between adjacent objects, the logic can be simplified.
Apply this diff to adjust the separator output:
let i = 0;
for (const object of objects) {
const url = urls[i];
- if (i > 0) console.log(options.separator);
try {
if (object == null) {
spinner.fail(`Failed to fetch object: ${colors.red(url)}.`);
if (authLoader == null) {
console.error(
"It may be a private object. Try with -a/--authorized-fetch.",
);
}
success = false;
+ } else {
+ spinner.succeed(`Fetched object: ${colors.green(url)}.`);
+ if (options.raw) {
+ printJson(await object.toJsonLd({ contextLoader }));
+ } else if (options.compact) {
+ printJson(
+ await object.toJsonLd({ format: "compact", contextLoader }),
+ );
+ } else if (options.expand) {
+ printJson(
+ await object.toJsonLd({ format: "expand", contextLoader }),
+ );
+ } else {
+ console.log(object);
+ }
+ }
+ if (i < urls.length - 1) {
+ console.log(options.separator);
+ }
}
- if (i < urls.length - 1) {
- console.log(options.separator);
- }
} catch (_) {
success = false;
}
+ i++;
}This change moves the separator output to after each object's processing, only if there are more objects to process.
📝 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.
| for (const object of objects) { | |
| const url = urls[i]; | |
| if (i > 0) console.log(options.separator); | |
| i++; | |
| try { | |
| if (object == null) { | |
| spinner.fail(`Failed to fetch object: ${colors.red(url)}.`); | |
| if (authLoader == null) { | |
| console.error( | |
| "It may be a private object. Try with -a/--authorized-fetch.", | |
| ); | |
| } | |
| success = false; | |
| } else { | |
| spinner.succeed(`Fetched object: ${colors.green(url)}.`); | |
| if (options.raw) { | |
| printJson(await object.toJsonLd({ contextLoader })); | |
| } else if (options.compact) { | |
| printJson( | |
| await object.toJsonLd({ format: "compact", contextLoader }), | |
| ); | |
| } else if (options.expand) { | |
| printJson( | |
| await object.toJsonLd({ format: "expand", contextLoader }), | |
| ); | |
| } else { | |
| console.log(object); | |
| } | |
| if (i < urls.length - 1) { | |
| console.log(options.separator); | |
| } | |
| for (const object of objects) { | |
| const url = urls[i]; | |
| try { | |
| if (object == null) { | |
| spinner.fail(`Failed to fetch object: ${colors.red(url)}.`); | |
| if (authLoader == null) { | |
| console.error( | |
| "It may be a private object. Try with -a/--authorized-fetch.", | |
| ); | |
| } | |
| success = false; | |
| } else { | |
| spinner.succeed(`Fetched object: ${colors.green(url)}.`); | |
| if (options.raw) { | |
| printJson(await object.toJsonLd({ contextLoader })); | |
| } else if (options.compact) { | |
| printJson( | |
| await object.toJsonLd({ format: "compact", contextLoader }), | |
| ); | |
| } else if (options.expand) { | |
| printJson( | |
| await object.toJsonLd({ format: "expand", contextLoader }), | |
| ); | |
| } else { | |
| console.log(object); | |
| } | |
| } | |
| if (i < urls.length - 1) { | |
| console.log(options.separator); | |
| } | |
| } catch (_) { | |
| success = false; | |
| } | |
| i++; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
CHANGES.md (1)
39-40: Fix indentation in changelog entries.The indentation of these list items is inconsistent with the rest of the file (using 5 spaces instead of 2 spaces).
Apply this diff to fix the indentation:
- - Added `-u`/`--user-agent` option to `fedify lookup` subcommand. - - Added `-u`/`--user-agent` option to `fedify node` subcommand. + - Added `-u`/`--user-agent` option to `fedify lookup` subcommand. + - Added `-u`/`--user-agent` option to `fedify node` subcommand. - - Added `fedifyHook()` function. + - Added `fedifyHook()` function.Also applies to: 64-67
🧰 Tools
🪛 Markdownlint (0.35.0)
39-39: Expected: 2; Actual: 5
Unordered list indentation(MD007, ul-indent)
40-40: Expected: 2; Actual: 5
Unordered list indentation(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGES.md(4 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGES.md
39-39: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
40-40: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
64-64: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
67-67: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
69-69: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
72-72: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
75-75: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
92-92: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
99-99: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
306-306: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
308-308: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
301-301: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
CHANGES.md
Outdated
| Version 1.2.8 | ||
| ------------- | ||
|
|
||
| To be released. |
There was a problem hiding this comment.
Remove duplicate version section.
There are two "Version 1.2.8" sections. One is marked as "To be released" while the other has a release date. This appears to be a mistake.
Remove the duplicate section:
-Version 1.2.8
--------------
-
-To be released.📝 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.
| Version 1.2.8 | |
| ------------- | |
| To be released. |
🧰 Tools
🪛 Markdownlint (0.35.0)
99-99: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
CHANGES.md (1)
75-77: Consider expanding the changelog entry.The current entry could be more descriptive about the specific changes related to AbortController. Consider adding details about:
- Which interfaces were updated to include the signal parameter
- The purpose of adding AbortController (e.g., for request timeout control)
- Any new options or configurations added
Example expansion:
- - Added options related to `AbortController`. - [[#51] [#187] by PDJ] + - Added support for request cancellation using `AbortController`: + - Added optional `signal` parameter to `GetDocumentLoaderOptions`, `GetActorHandleOptions`, + `LookupObjectOptions`, and `LookupWebFingerOptions` interfaces + - Enables control over maximum request duration for outgoing API calls + - Allows graceful cancellation of in-flight requests + [[#51] [#187] by PDJ]🧰 Tools
🪛 Markdownlint (0.35.0)
75-75: Expected: 0; Actual: 1
Unordered list indentation(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
CHANGES.md(3 hunks)src/runtime/docloader.ts(3 hunks)src/vocab/actor.ts(4 hunks)src/vocab/lookup.ts(2 hunks)src/webfinger/lookup.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/runtime/docloader.ts
- src/vocab/actor.ts
- src/vocab/lookup.ts
- src/webfinger/lookup.ts
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGES.md
75-75: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
| @@ -72,6 +72,9 @@ To be released. | |||
| - Let the `fedify lookup` command take multiple arguments. | |||
| [[#173], [#186] by PGD] | |||
|
|
|||
| - Added options related to `AbortController`. | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix list indentation.
The indentation of the bullet point doesn't match the style of other entries in the changelog.
Apply this diff to fix the indentation:
- - Added options related to `AbortController`.
+ - Added options related to `AbortController`.📝 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.
| - Added options related to `AbortController`. | |
| - Added options related to `AbortController`. |
🧰 Tools
🪛 Markdownlint (0.35.0)
75-75: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGES.md (1)
75-77: Enhance the changelog entry with more details.The current changelog entry for AbortController could be more informative. Consider expanding it to include:
- List the specific interfaces that were updated (e.g.,
GetDocumentLoaderOptions,GetActorHandleOptions,LookupObjectOptions,LookupWebFingerOptions)- Explain that this addition enables request timeout control
- Add a brief example of how to use the new
signalparameterApply this diff to enhance the changelog entry:
- - Added options related to `AbortController`. - [[#51] [#187] by PDJ] + - Added support for request cancellation using `AbortController`: + [[#51] [#187] by PDJ] + + - Added optional `signal` parameter to the following interfaces for request timeout control: + - `GetDocumentLoaderOptions` + - `GetActorHandleOptions` + - `LookupObjectOptions` + - `LookupWebFingerOptions` + + - Example usage: + ```typescript + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 5000); // 5 second timeout + const options = { signal: controller.signal }; + ```🧰 Tools
🪛 Markdownlint (0.35.0)
75-75: Expected: 0; Actual: 1
Unordered list indentation(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGES.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGES.md
75-75: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (27)
src/vocab/type.ts (1)
22-87: Consolidate function documentation.The documentation is duplicated across all overload signatures. In TypeScript, it's recommended to document only the implementation signature and remove documentation from overload signatures.
export function getTypeId(object: Object | Link): URL; - -/** - * Returns the type URI of the given object. - * - * @example - * ``` typescript - * import { getTypeId, Person } from "@fedify/fedify"; - * - * const obj = new Person({}); - * console.log(getTypeId(obj)); - * // => new URL("https://www.w3.org/ns/activitystreams#Person") - * ``` - * - * @param object The Activity Vocabulary object. - * @returns The type URI of the object, e.g., - * `new URL("https://www.w3.org/ns/activitystreams#Person")`. - * If the given `object` is `null` or `undefined`, returns `null` or - * `undefined`, respectively. - * @since 1.3.0 - */ export function getTypeId(object: Object | Link | undefined): URL | undefined; - -/** - * Returns the type URI of the given object. - * - * @example - * ``` typescript - * import { getTypeId, Person } from "@fedify/fedify"; - * - * const obj = new Person({}); - * console.log(getTypeId(obj)); - * // => new URL("https://www.w3.org/ns/activitystreams#Person") - * ``` - * - * @param object The Activity Vocabulary object. - * @returns The type URI of the object, e.g., - * `new URL("https://www.w3.org/ns/activitystreams#Person")`. - * If the given `object` is `null` or `undefined`, returns `null` or - * `undefined`, respectively. - * @since 1.3.0 - */ export function getTypeId(object: Object | Link | null): URL | null;src/webfinger/lookup.ts (2)
30-40: LGTM: Interface changes are well documented and typed correctlyThe new properties are well documented and follow TypeScript best practices. The signal type is correctly specified as AbortSignal.
Consider implementing a timeout wrapper utility that creates an AbortSignal with a timeout, making it easier for consumers to implement request timeouts. Example usage could be:
const options: LookupWebFingerOptions = { signal: createTimeoutSignal(5000), // 5 second timeout };
Line range hint
90-116: Add safety measures for redirect handlingWhile the signal implementation is correct, the redirect handling in the while loop could potentially continue indefinitely. Consider adding a maximum redirect limit.
Here's a suggested implementation:
+ const MAX_REDIRECTS = 10; + let redirectCount = 0; while (true) { + if (redirectCount >= MAX_REDIRECTS) { + logger.debug( + "Maximum redirect limit reached", + { url: url.href, limit: MAX_REDIRECTS } + ); + return null; + } logger.debug("Fetching WebFinger resource descriptor from {url}...", { url: url.href } ); // ... existing code ... if (response.status >= 300 && response.status < 400 && response.headers.has("Location")) { + redirectCount++; url = new URL( response.headers.get("Location")!, response.url == null || response.url === "" ? url : response.url ); continue; }src/vocab/actor.ts (1)
138-166: Consider enhancing error attributes in spanThe OpenTelemetry implementation is solid, but consider adding more error details to the span attributes when an error occurs.
} catch (error) { span.setStatus({ code: SpanStatusCode.ERROR, message: String(error), }); + span.setAttribute('error.type', error.name); + span.setAttribute('error.message', error.message); throw error; }docs/manual/opentelemetry.md (6)
24-28: Improve clarity in setup instructionsConsider revising the redundant phrasing to be more concise:
-To trace your Fedify application with OpenTelemetry, you need to set up the -OpenTelemetry SDK. First of all, you need to install the OpenTelemetry SDK and -the tracer exporter you want to use. For example, if you want to use the trace -exporter for OTLP (http/protobuf), you should install the following packages: +To trace your Fedify application with OpenTelemetry, set up the OpenTelemetry SDK +and install your preferred tracer exporter. For the OTLP (http/protobuf) trace +exporter, install these packages:🧰 Tools
🪛 LanguageTool
[style] ~25-~25: Often, this adverbial phrase is redundant. Consider using an alternative.
Context: ... need to set up the OpenTelemetry SDK. First of all, you need to install the OpenTelemetry ...(FIRST_OF_ALL)
[style] ~26-~26: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...r you want to use. For example, if you want to use the trace exporter for OTLP (http/p...(REP_WANT_TO_VB)
55-58: Add context for OTLP configurationConsider adding a brief note explaining the default OTLP endpoint (
http://localhost:4317) and when to modify the headers.
78-81: Enhance Sentry integration exampleConsider adding a brief explanation of why one might choose Sentry as a trace exporter (e.g., error tracking, performance monitoring, etc.) to help users make an informed decision.
120-133: Enhance span documentation with examplesConsider adding example scenarios or sample traces for each span type to help users understand when these spans are created and how they relate to each other in a typical request flow.
158-158: Fix grammatical error in attribute descriptionCorrect the agreement error in the description:
-The URI(s) of the original object to which the object reply. +The URI(s) of the original object to which the object replies.🧰 Tools
🪛 LanguageTool
[grammar] ~158-~158: Possible agreement error - use third-person verb forms for singular and mass nouns.
Context: ...the original object to which the object reply. | `["https...(MASS_AGREEMENT)
147-173: Consider grouping related attributesThe attributes table is comprehensive but could be more organized. Consider grouping related attributes (e.g., all signature-related attributes together) and adding subsection headers in the table for better readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~158-~158: Possible agreement error - use third-person verb forms for singular and mass nouns.
Context: ...the original object to which the object reply. | `["https...(MASS_AGREEMENT)
src/runtime/docloader.ts (2)
Line range hint
292-301: Critical: Signal not propagated to redirected requestsThe
signalis correctly passed to the initial fetch call, but when handling redirects, the signal is not propagated to the recursiveloadcall. This could result in redirected requests continuing even after the original request is aborted.Apply this fix to ensure proper signal propagation:
if ( response.status >= 300 && response.status < 400 && response.headers.has("Location") ) { - return load(response.headers.get("Location")!); + return load(response.headers.get("Location")!, signal); }Additionally, update the load function signature to accept the signal:
- async function load(url: string): Promise<RemoteDocument> { + async function load(url: string, signal?: AbortSignal | null): Promise<RemoteDocument> {
Line range hint
374-397: Critical: Add signal support to authenticated document loaderThe
getAuthenticatedDocumentLoaderfunction lacks signal support, which means authenticated requests cannot be cancelled. This should be implemented for consistency withgetDocumentLoader.Apply these changes to add signal support:
export interface GetAuthenticatedDocumentLoaderOptions { allowPrivateAddress?: boolean; userAgent?: GetUserAgentOptions | string; + signal?: AbortSignal | null; } export function getAuthenticatedDocumentLoader( identity: { keyId: URL; privateKey: CryptoKey }, - { allowPrivateAddress, userAgent }: GetAuthenticatedDocumentLoaderOptions = {}, + { allowPrivateAddress, userAgent, signal }: GetAuthenticatedDocumentLoaderOptions = {}, ): DocumentLoader { validateCryptoKey(identity.privateKey); - async function load(url: string): Promise<RemoteDocument> { + async function load(url: string, signal?: AbortSignal | null): Promise<RemoteDocument> { // ... existing code ... const response = await fetch(request, { redirect: "manual", + signal, }); if ( response.status >= 300 && response.status < 400 && response.headers.has("Location") ) { - return load(response.headers.get("Location")!); + return load(response.headers.get("Location")!, signal); } // ... rest of the code ... } return load; }docs/manual/federation.md (3)
343-343: Maintain consistent heading styleFor consistency with other sections in the document, use setext-style headings (with
===or---underlines) instead of ATX-style headings (with#).-### `tracerProvider` +`tracerProvider` +----------------🧰 Tools
🪛 Markdownlint (0.35.0)
343-343: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
343-354: Add code example for custom tracer provider configurationConsider adding a code example to demonstrate how to configure a custom tracer provider, similar to other sections in the documentation.
Add this example after line 354:
For example: ~~~~ typescript import { createFederation } from "@fedify/fedify"; import { NodeTracerProvider } from "@opentelemetry/sdk-trace-node"; const tracerProvider = new NodeTracerProvider(); tracerProvider.register(); const federation = createFederation({ // ... other options tracerProvider, }); ~~~~🧰 Tools
🪛 Markdownlint (0.35.0)
343-343: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
Line range hint
1-1: Document the new AbortController featureThe PR implements AbortController for API requests, but the documentation doesn't cover this important feature. Consider adding a new section that explains:
- How to use AbortController with the Federation API
- Which methods support request cancellation
- Example usage with timeouts
Here's a suggested addition to the documentation:
Request Cancellation ------------------ *This API is available since Fedify 1.3.0.* The Federation API supports request cancellation using `AbortController` for various operations that make outgoing HTTP requests. This allows you to set timeouts or manually cancel long-running requests. The following interfaces support request cancellation through their `signal` parameter: - `GetDocumentLoaderOptions` - `GetActorHandleOptions` - `LookupObjectOptions` - `LookupWebFingerOptions` Example usage with a timeout: ~~~~ typescript import { createFederation } from "@fedify/fedify"; const federation = createFederation({ // ... other options }); // Create an AbortController with a 5-second timeout const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 5000); try { const actor = await federation.getActorHandle("@[email protected]", { signal: controller.signal, }); // Process the actor... } catch (error) { if (error.name === "AbortError") { console.log("Request timed out"); } } finally { clearTimeout(timeout); } ~~~~🧰 Tools
🪛 Markdownlint (0.35.0)
343-343: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
src/webfinger/handler.ts (7)
62-64: Handle the case where bothoptions.tracerandoptions.spanare undefinedIf both
tracerandspanare undefined inoptions, the function may not properly trace the request. Ensure that the absence of atracerdoesn't lead to missing spans if aspanis provided.Consider updating the condition to check for
options.spanas well:if (options.tracer == null && options.span == null) { return await handleWebFingerInternal(request, options); }
88-95: Clarify the dependency onactorDispatcherinhandleWebFingerInternalIn
handleWebFingerInternal, there are two checks foractorDispatcherbeingnull. The first check occurs at line 90, and the second at line 116. This could be streamlined to avoid redundant code.Refactor the code to remove the redundant check:
async function handleWebFingerInternal<TContextData>( request: Request, { context, actorDispatcher, actorHandleMapper, onNotFound, span, }: WebFingerHandlerParameters<TContextData>, ): Promise<Response> { - if (actorDispatcher == null) return await onNotFound(request); const resource = context.url.searchParams.get("resource"); if (resource == null) { return new Response("Missing resource parameter.", { status: 400 }); } span?.setAttribute("webfinger.resource", resource); // Remove the second check for actorDispatcher
113-116: EnsureresourceUrl.protocolhandles all cases correctlyWhen setting the
webfinger.resource.schemeattribute, theresourceUrl.protocol.replace(/:$/, "")operation assumes that the protocol ends with a colon. While this is standard, ensure that all possible cases are handled, especially with non-standard URLs.Consider using the
resourceUrl.schemeproperty if available, or adding a safety check:span?.setAttribute( "webfinger.resource.scheme", - resourceUrl.protocol.replace(/:$/, ""), + resourceUrl.protocol.endsWith(':') ? resourceUrl.protocol.slice(0, -1) : resourceUrl.protocol, );
2-2: Organize imports to comply with project conventionsThe import statement for
SpanandTraceris added separately. For better readability and maintenance, group related imports together.Organize the import statements:
-import { getLogger } from "@logtape/logtape"; +import { getLogger } from "@logtape/logtape"; import type { Span, Tracer } from "@opentelemetry/api"; import { SpanKind, SpanStatusCode } from "@opentelemetry/api";
40-48: Document the newtracerandspanparameters in the interfaceThe
WebFingerHandlerParametersinterface now includestracerandspan, but their purpose isn't fully explained in the comments. Providing clear documentation will help other developers understand how to use these parameters.Enhance the documentation for
tracerandspan:/** * The OpenTelemetry tracer used for tracing the request. Optional. */ tracer?: Tracer; /** * The OpenTelemetry span representing the current operation. Optional. */ span?: Span;
62-64: Consider defaulting to a no-op tracer to simplify code pathsBy defaulting
tracerto a no-op implementation when it's not provided, you can simplify the conditional logic and ensure that tracing calls are safe to invoke without null checks.Implement a no-op tracer or use OpenTelemetry's default:
import { trace, context } from "@opentelemetry/api"; const tracer = options.tracer ?? trace.getTracer("default"); return await tracer.startActiveSpan( // rest of the code );
70-74: Handle non-OK responses appropriately in tracingWhen setting the span status based on
response.ok, consider capturing more detailed information for debugging purposes, such as the response status code or error messages.Enhance the span status setting:
span.setStatus({ code: response.ok ? SpanStatusCode.UNSET : SpanStatusCode.ERROR, + message: response.ok ? undefined : `HTTP ${response.status} ${response.statusText}`, });src/sig/proof.ts (2)
13-13: Avoid Shadowing the GlobalObjectImporting
type Objectshadows the globalObjecttype in JavaScript, which can lead to confusion and potential bugs. Consider renaming the imported type to prevent this shadowing.For example, you could rename
ObjecttoAPObjector another descriptive name that doesn't conflict with global types.🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Do not shadow the global "Object" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
148-150: Consider Moving Attribute Setting Insidespan.isRecording()CheckTo ensure consistency and avoid unnecessary operations when the span is not recording, consider moving the setting of the
activitypub.object.idattribute inside theif (span.isRecording())block.Apply this diff to adjust the code:
- if (object.id != null) { - span.setAttribute("activitypub.object.id", object.id.href); - } + if (span.isRecording()) { + if (object.id != null) { + span.setAttribute("activitypub.object.id", object.id.href); + } + }src/sig/ld.ts (2)
144-177: Consider Abstracting Tracing Logic to Reduce DuplicationThe tracing implementation in
signJsonLdandverifyJsonLdfunctions shares similar patterns. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting the tracing setup into a common helper function or utility. This refactor will reduce code duplication and simplify future updates.Also applies to: 375-448
153-158: Pass Only Relevant Options tocreateSignatureWhen calling
createSignature, it's best practice to pass only the necessary options to prevent unintended side effects. Extract the relevant properties instead of passing the entireoptionsobject.Apply this diff:
const signature = await createSignature( jsonLd, privateKey, keyId, - options, + { + contextLoader: options.contextLoader, + created: options.created, + }, );src/federation/middleware.ts (1)
1965-1966: Consider Encapsulating Tracing Parameters to Simplify Method SignaturesThe addition of
tracerandtracerProviderparameters to multiple method signatures increases the number of parameters, which may affect readability and maintainability. Consider encapsulating these tracing-related parameters into a singleoptionsobject or utilizing a context pattern to reduce parameter clutter and enhance code clarity.Also applies to: 1984-1984, 2082-2082, 2651-2651, 2900-2900
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
CHANGES.md(2 hunks)docs/.vitepress/config.mts(1 hunks)docs/manual/federation.md(1 hunks)docs/manual/opentelemetry.md(1 hunks)docs/package.json(2 hunks)src/federation/handler.ts(6 hunks)src/federation/middleware.ts(12 hunks)src/federation/send.ts(4 hunks)src/runtime/docloader.ts(3 hunks)src/sig/http.ts(8 hunks)src/sig/ld.ts(5 hunks)src/sig/proof.ts(5 hunks)src/vocab/actor.ts(5 hunks)src/vocab/lookup.ts(4 hunks)src/vocab/mod.ts(2 hunks)src/vocab/type.test.ts(1 hunks)src/vocab/type.ts(1 hunks)src/webfinger/handler.ts(4 hunks)src/webfinger/lookup.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/vocab/lookup.ts
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGES.md
60-60: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
62-62: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
64-64: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
65-65: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
66-66: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
67-67: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
68-68: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
69-69: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
70-70: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
71-71: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
72-72: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
73-73: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
74-74: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
75-75: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
88-88: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
docs/manual/federation.md
343-343: Expected: setext; Actual: atx
Heading style
(MD003, heading-style)
🪛 LanguageTool
docs/manual/opentelemetry.md
[style] ~25-~25: Often, this adverbial phrase is redundant. Consider using an alternative.
Context: ... need to set up the OpenTelemetry SDK. First of all, you need to install the OpenTelemetry ...
(FIRST_OF_ALL)
[style] ~26-~26: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...r you want to use. For example, if you want to use the trace exporter for OTLP (http/p...
(REP_WANT_TO_VB)
[style] ~65-~65: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ode which sets up the OpenTelemetry SDK needs to be executed before > the Fedify server ...
(REP_NEED_TO_VB)
[grammar] ~158-~158: Possible agreement error - use third-person verb forms for singular and mass nouns.
Context: ...the original object to which the object reply. | `["https...
(MASS_AGREEMENT)
🪛 Biome (1.9.4)
src/sig/proof.ts
[error] 13-13: Do not shadow the global "Object" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/vocab/type.ts
[error] 1-1: Do not shadow the global "Object" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (42)
src/vocab/type.test.ts (3)
1-4: LGTM! Import statements are well-organized.
The imports are properly structured, following the convention of external dependencies first followed by internal modules. All imports are utilized in the test file.
6-20: Consider restructuring tests for better organization and coverage.
While the current tests cover the basic scenarios, consider the following improvements:
- Split scenarios into separate test cases for better clarity and maintenance:
-test("getTypeId()", () => {
+test("getTypeId() with valid Person object returns correct type URL", () => {
const obj = new Person({});
assertEquals(
getTypeId(obj),
new URL("https://www.w3.org/ns/activitystreams#Person"),
);
+});
+
+test("getTypeId() with null values returns null", () => {
const obj2: Person | null = null;
assertEquals(getTypeId(obj2), null);
+ const obj4: Person | null | undefined = null;
+ assertEquals(getTypeId(obj4), null);
+});
+
+test("getTypeId() with undefined values returns undefined", () => {
const obj3: Person | undefined = undefined;
assertEquals(getTypeId(obj3), undefined);
- const obj4: Person | null | undefined = null;
- assertEquals(getTypeId(obj4), null);
const obj5: Person | null | undefined = undefined;
assertEquals(getTypeId(obj5), undefined);
});- Add test cases for invalid scenarios:
6-20: Verify test coverage for all scenarios.
Let's verify if all possible scenarios from the implementation are covered in tests:
✅ Verification successful
Test coverage is complete and accurate
The current test coverage is comprehensive and matches all code paths in the implementation:
- Tests null and undefined cases which align with the
if (object == null)check - Tests the main case with a Person object, verifying the correct type ID URL is returned
- Covers all function overloads (Object|Link, Object|Link|null, Object|Link|undefined, Object|Link|null|undefined)
- The implementation is simple with only two paths: null check and type ID retrieval
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for other test files that might contain getTypeId tests
fd -e test.ts -x rg -l "getTypeId"
# Check the implementation for all possible code paths
ast-grep --pattern 'export function getTypeId($_) {
$$$
}'
# Look for usage patterns to identify untested scenarios
rg "getTypeId\(" --type ts
Length of output: 1427
Script:
#!/bin/bash
# Let's check the implementation details of getTypeId to verify test coverage
ast-grep --pattern 'export function getTypeId($_) {
$$$
}' src/vocab/type.ts
# Let's also check the type definitions to understand all possible input types
rg "type Object|type Link" --type ts -A 3
Length of output: 1632
Script:
#!/bin/bash
# Let's get the actual implementation of getTypeId function to see all code paths
rg "export function getTypeId" -A 10 src/vocab/type.ts
# Also check if there are any specific type checks or conditions in the implementation
rg "instanceof|typeof" src/vocab/type.ts
Length of output: 1301
docs/package.json (2)
6-6: Verify compatibility with the new fedify version
The significant version jump (dev.508 -> dev.557) could introduce breaking changes. Ensure that the new version is compatible with the AbortController implementation.
Let's check the changelog between these versions:
✅ Verification successful
Based on the search results, I can see that AbortController and signal-related functionality is a well-established feature in the codebase, with proper implementations across various modules. Let me search for any breaking changes between these versions:
Based on the search results, I can now provide a final response:
The new fedify version appears compatible with AbortController implementation
The changes between versions primarily involve message queue configuration improvements, with no breaking changes to the AbortController functionality. The AbortController/signal implementation is well-established across the codebase and the version update (1.3.0-dev.557) maintains backward compatibility for this feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for any breaking changes or AbortController-related updates
rg -A 5 "BREAKING CHANGE|AbortController|signal" $(fd -e md -e txt changelog CHANGELOG)
Length of output: 13369
Script:
#!/bin/bash
# Look for version history or changelog entries
rg -i "version 1.3.0|1\.3\.0-dev" -A 10
Length of output: 3164
39-41: Clarify the scope of telemetry additions
The addition of OpenTelemetry and Sentry dependencies appears to extend beyond the PR's stated objective of implementing AbortController for API timeouts. While these are valuable monitoring tools, their inclusion should be documented and justified.
Let's check if these changes are documented:
✅ Verification successful
OpenTelemetry and Sentry additions are well-documented and part of a planned feature
The telemetry additions are properly documented and justified:
- CHANGES.md explicitly mentions OpenTelemetry support as a new feature (More built-in OpenTelemetry instrumentations #170)
- Comprehensive documentation exists in docs/manual/opentelemetry.md explaining the setup and usage
- The implementation is thorough with traces across multiple components (federation, webfinger, vocab, etc.)
- Sentry integration is documented as an optional trace exporter in the OpenTelemetry documentation
These changes are part of a planned observability feature rather than scope creep, and the documentation properly covers their integration and usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check changelog and documentation for telemetry additions
fd -e md -e txt changelog CHANGELOG
Length of output: 134
Script:
#!/bin/bash
# Let's try different approaches to find documentation about telemetry
# 1. Look for any markdown files that might contain documentation
fd -e md
# 2. Search for telemetry-related terms in all text files
rg -i "telemetry|opentelemetry|sentry" -g "!{package-lock.json,yarn.lock,pnpm-lock.yaml}"
# 3. Check if there are any configuration files for these services
fd -g "*telemetry*"
fd -g "*sentry*"
Length of output: 7928
src/vocab/mod.ts (2)
10-10: LGTM! Documentation fix looks good.
The spelling correction improves the documentation quality.
55-55: Verify the relevance of this change to the PR objectives.
The addition of type.ts export seems unrelated to the PR's stated objective of implementing AbortController for API requests. Could you clarify how this change supports the implementation of request timeouts?
Let's check if the exported module contains any abort-related functionality:
src/vocab/type.ts (2)
3-21: Documentation looks great!
The documentation is thorough, includes clear examples, and properly documents the behavior for edge cases.
1-99: Verify if this file is within PR scope.
This file introduces a new getTypeId function, but the PR's objective is to implement AbortController for API requests. Please clarify if this change is intentionally part of this PR or should be moved to a separate PR.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Do not shadow the global "Object" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/webfinger/lookup.ts (2)
2-7: LGTM: OpenTelemetry imports are well structured
The imports are correctly specified and follow TypeScript best practices.
52-88: LGTM: Tracing implementation follows best practices
The tracing implementation is well structured with proper error handling and span management.
Let's verify that all error paths are properly traced:
✅ Verification successful
Let me verify the success paths as well to ensure complete tracing coverage.
Tracing implementation is complete and correct
The error and success paths are properly handled in the tracing implementation:
- Error paths consistently set
SpanStatusCode.ERRORwith error messages - Success paths appropriately set status based on the result (
null->ERROR, valid result ->OK) - The span is always ended in the
finallyblock - The implementation aligns with similar patterns used throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all error handling paths to ensure they set span status
ast-grep --pattern 'span.setStatus({
code: SpanStatusCode.$_,
$$$
})'
Length of output: 2207
Script:
#!/bin/bash
# Search for success path span status settings
ast-grep --pattern 'span.setStatus({
code: SpanStatusCode.OK,
$$$
})'
# Also search for any direct span.setStatus calls to ensure we haven't missed any patterns
rg "span\.setStatus" -A 2
Length of output: 2965
src/federation/send.ts (2)
2-2: LGTM: Import statement is correctly added
The TracerProvider import from OpenTelemetry is properly added to support the new tracing functionality.
1-1: Verify AbortController implementation across the codebase
Let's ensure that AbortController implementation is consistent across all API calls.
docs/.vitepress/config.mts (1)
73-73: LGTM! Verify the OpenTelemetry documentation file exists.
The addition of OpenTelemetry documentation to the manual section is well-placed and follows the existing structure.
Let's verify that the referenced documentation file exists:
✅ Verification successful
Documentation link verified and exists
The OpenTelemetry documentation file exists at the expected path docs/manual/opentelemetry.md, confirming that the navigation link in the VitePress config is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the OpenTelemetry documentation file exists
# Expected: File should exist at docs/manual/opentelemetry.md
if [ -f "docs/manual/opentelemetry.md" ]; then
echo "✓ OpenTelemetry documentation file exists"
else
echo "✗ Missing documentation file: docs/manual/opentelemetry.md"
fi
Length of output: 134
src/vocab/actor.ts (4)
1-1: LGTM: OpenTelemetry import added correctly
The addition of OpenTelemetry imports is well-structured and necessary for the new tracing functionality.
97-108: LGTM: Well-documented interface extensions
The additions of signal and tracerProvider properties are well-documented and properly typed, providing clear functionality for request cancellation and tracing.
174-187: LGTM: Robust error handling for aborted requests
The implementation properly handles request abortion with clear error messages and correct error propagation.
231-238: LGTM: Consistent parameter propagation
The function correctly propagates both the tracer provider and abort signal to the underlying lookupWebFinger call.
src/runtime/docloader.ts (3)
238-242: LGTM! Signal parameter correctly implemented.
The optional signal parameter is properly typed as AbortSignal | null which matches the fetch API's expectations.
263-264: LGTM! Function signature properly updated.
The signal parameter is correctly destructured from the options object, maintaining consistency with the interface.
238-242: Verify the impact of AbortSignal changes
Let's verify the usage of these document loaders across the codebase to ensure consistent implementation of request cancellation.
✅ Verification successful
Let me try a more focused search to verify the impact of AbortSignal changes.
AbortSignal implementation is consistent across the codebase
The AbortSignal changes in docloader.ts align with the existing patterns in the codebase:
- The
signal?: AbortSignal | nulltype signature is consistently used across multiple modules including webfinger/lookup.ts, vocab/actor.ts, and federation/mq.ts - AbortController usage is properly implemented in test files and documented examples
- The changes maintain consistency with the existing request cancellation patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of document loaders to verify signal implementation
# Check for direct usage of the document loaders
echo "Checking direct usage of document loaders..."
rg -A 5 "getDocumentLoader|getAuthenticatedDocumentLoader"
# Check for existing timeout or cancellation patterns
echo "Checking existing timeout patterns..."
rg -A 5 "setTimeout.*fetch|new Promise.*timeout|AbortController"
# Check for any TODOs related to request timeouts
echo "Checking timeout-related TODOs..."
rg -i "todo.*timeout"
Length of output: 159389
Script:
#!/bin/bash
# Search for AbortSignal usage and related patterns
echo "Checking AbortSignal usage..."
rg -A 3 "AbortSignal|signal\?:"
echo "Checking fetch with signal..."
rg -A 3 "fetch.*signal"
echo "Checking for request cancellation patterns..."
rg -A 3 "abort\(\)|new AbortController"
Length of output: 4857
CHANGES.md (2)
60-75: Skip: List indentation issues already addressed.
The indentation issues have already been identified in past review comments.
Also applies to: 88-88
🧰 Tools
🪛 Markdownlint (0.35.0)
60-60: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
62-62: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
64-64: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
65-65: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
66-66: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
67-67: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
68-68: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
69-69: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
70-70: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
71-71: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
72-72: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
73-73: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
74-74: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
75-75: Expected: 2; Actual: 5
Unordered list indentation
(MD007, ul-indent)
94-97: LGTM: Link references are properly formatted.
The link references follow the established format and are correctly added.
src/sig/proof.ts (2)
139-184: Tracing Implementation in signObject Function is Correct
The integration of OpenTelemetry tracing into the signObject function is well-implemented. The code correctly initializes the tracer, starts an active span, sets relevant attributes, handles errors appropriately, and ensures the span is ended in the finally block.
230-271: Tracing Implementation in verifyProof Function is Correct
The addition of OpenTelemetry tracing in the verifyProof function is properly handled. The code accurately starts an active span, captures relevant attributes, manages error states with appropriate span status updates, and ensures the span is ended correctly.
src/sig/http.ts (2)
Line range hint 85-109: Ensure consistent 'isRecording()' checks before setting span attributes
In signRequestInternal, attributes are set within an if (span.isRecording()) check. This is good practice to avoid unnecessary operations when the span is not recording. Ensure this pattern is consistently applied in other functions as well.
16-16: Verify compatibility of 'with { type: "json" }' syntax in import statement
The import statement import metadata from "../deno.json" with { type: "json" }; uses syntax that may not be compatible with all TypeScript configurations or environments. Ensure that this syntax is supported in your project's build and runtime environment.
Run the following script to check for TypeScript compiler support:
✅ Verification successful
Import syntax is compatible with project configuration
The project's TypeScript configuration explicitly supports this syntax through:
"target": "ESNext"and"module": "ESNext"which enable latest JavaScript features"moduleResolution": "bundler"which supports import assertions"resolveJsonModule": true"which enables JSON module imports
The syntax import ... with { type: "json" } is part of the ECMAScript proposal for Import Assertions, which is supported in modern TypeScript when targeting ESNext, exactly as configured in this project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the TypeScript compiler supports JSON imports with 'with { type: "json" }' syntax.
# Test: Attempt to compile a test file with the import statement.
echo 'import metadata from "./test.json" with { type: "json" };' > test.ts
echo '{}' > test.json
tsc test.ts
# Expected result: No compilation errors.
Length of output: 186
Script:
#!/bin/bash
# Let's check the TypeScript configuration and package.json for compiler options and dependencies
fd "tsconfig.json|package.json" --type f --exec cat {}
Length of output: 5314
src/sig/ld.ts (1)
117-122: Consistent Documentation for tracerProvider Option
The addition of the optional tracerProvider property to both SignJsonLdOptions and VerifyJsonLdOptions interfaces is well-documented and includes the @since 1.3.0 tag. Ensure that this version aligns with the actual release version to maintain accurate documentation.
Also applies to: 354-359
src/federation/handler.ts (3)
363-363: Ensure optional 'tracerProvider' is handled correctly in 'InboxHandlerParameters'
Since tracerProvider is optional in InboxHandlerParameters, please ensure that any functions consuming this parameter handle cases when it is undefined to prevent potential runtime errors.
381-381: Confirm 'tracerProvider' is appropriately passed to 'handleInbox'
Verify that the handleInbox function signature includes the tracerProvider parameter and that it correctly manages scenarios where tracerProvider may be undefined.
463-463: Validate 'tracerProvider' usage in verification functions
Ensure that the verifyJsonLd, verifyObject, and verifyRequest functions accept the tracerProvider parameter and handle it properly, especially when it is undefined, to avoid any unexpected errors.
Also applies to: 480-480, 521-521
src/federation/middleware.ts (11)
7-7: Valid Import of Tracing Types
The types Tracer and TracerProvider are correctly imported from the OpenTelemetry API to support tracing functionality.
538-538: Passing TracerProvider to sendActivity
The tracerProvider is appropriately passed to the sendActivity function, enabling tracing of activity sending operations.
1779-1779: Integrating TracerProvider into signObject Function
The tracerProvider is correctly passed to the signObject function, enhancing tracing during the object signing process.
1805-1805: Adding TracerProvider to signJsonLd Function
The inclusion of tracerProvider in the signJsonLd function call enables tracing support during JSON-LD signing.
1914-1914: Propagating Span and Tracer to Internal #fetch Method
The span and tracer parameters are correctly passed to the #fetch method, ensuring that tracing spans are properly propagated during the fetch operation.
1965-1966: Updating #fetch Method Signature to Include Tracer
The #fetch method signature is appropriately updated to include the tracer parameter, allowing tracing within this internal method.
1984-1984: Passing Tracer to handleWebFinger Function
The tracer is correctly passed to the handleWebFinger function, integrating tracing into the WebFinger handling process.
2082-2082: Including TracerProvider in handleInbox Function Call
The tracerProvider is properly included in the handleInbox function call, enabling tracing during inbox processing.
2651-2651: Passing TracerProvider to lookupObject Function
The tracerProvider is correctly passed to the lookupObject function within the options, enhancing tracing capabilities during object lookup.
2900-2900: Enhancing verifyRequest with TracerProvider
The tracerProvider is passed to the verifyRequest function, enabling tracing during request verification.
3082-3082: Including TracerProvider in sendActivity Call within forwardActivity
The tracerProvider is correctly included in the sendActivity call inside the forwardActivity method, allowing tracing of forwarded activities.
src/vocab/type.ts
Outdated
| @@ -0,0 +1,99 @@ | |||
| import type { Link, Object } from "./vocab.ts"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename imported Object type to avoid shadowing.
The imported Object type shadows the global Object type, which could lead to confusion. Consider renaming it to something more specific like ActivityObject or VocabObject.
-import type { Link, Object } from "./vocab.ts";
+import type { Link, ActivityObject } from "./vocab.ts";Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Do not shadow the global "Object" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/vocab/type.ts
Outdated
| export function getTypeId( | ||
| object: Object | Link | undefined | null, | ||
| ): URL | undefined | null { | ||
| if (object == null) return object; | ||
| const cls = object.constructor as | ||
| & (new (...args: unknown[]) => Object | Link) | ||
| & { | ||
| typeId: URL; | ||
| }; | ||
| return cls.typeId; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding runtime safety checks.
The implementation assumes the constructor has a typeId property without runtime validation. This could lead to runtime errors if a class doesn't implement the expected interface.
export function getTypeId(
object: Object | Link | undefined | null,
): URL | undefined | null {
if (object == null) return object;
const cls = object.constructor as
& (new (...args: unknown[]) => Object | Link)
& {
typeId: URL;
};
+ if (!('typeId' in cls)) {
+ throw new TypeError(`Class ${cls.name} does not have a typeId property`);
+ }
return cls.typeId;
}📝 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.
| export function getTypeId( | |
| object: Object | Link | undefined | null, | |
| ): URL | undefined | null { | |
| if (object == null) return object; | |
| const cls = object.constructor as | |
| & (new (...args: unknown[]) => Object | Link) | |
| & { | |
| typeId: URL; | |
| }; | |
| return cls.typeId; | |
| } | |
| export function getTypeId( | |
| object: Object | Link | undefined | null, | |
| ): URL | undefined | null { | |
| if (object == null) return object; | |
| const cls = object.constructor as | |
| & (new (...args: unknown[]) => Object | Link) | |
| & { | |
| typeId: URL; | |
| }; | |
| if (!('typeId' in cls)) { | |
| throw new TypeError(`Class ${cls.name} does not have a typeId property`); | |
| } | |
| return cls.typeId; | |
| } |
src/federation/send.ts
Outdated
|
|
||
| /** | ||
| * The tracer provider for tracing the request. | ||
| * If omitted, the global tracer provider is used. | ||
| * @since 1.3.0 | ||
| */ | ||
| tracerProvider?: TracerProvider; |
There was a problem hiding this comment.
Add AbortSignal parameter for request cancellation
While the tracerProvider addition is good, the interface is missing the signal parameter needed to implement the PR's main objective of supporting request cancellation via AbortController.
Consider adding the signal parameter:
export interface SendActivityParameters {
// ... existing parameters ...
/**
* The tracer provider for tracing the request.
* If omitted, the global tracer provider is used.
* @since 1.3.0
*/
tracerProvider?: TracerProvider;
+ /**
+ * An AbortSignal to cancel the request.
+ * @since 1.3.0
+ */
+ signal?: AbortSignal;
}📝 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.
| /** | |
| * The tracer provider for tracing the request. | |
| * If omitted, the global tracer provider is used. | |
| * @since 1.3.0 | |
| */ | |
| tracerProvider?: TracerProvider; | |
| /** | |
| * The tracer provider for tracing the request. | |
| * If omitted, the global tracer provider is used. | |
| * @since 1.3.0 | |
| */ | |
| tracerProvider?: TracerProvider; | |
| /** | |
| * An AbortSignal to cancel the request. | |
| * @since 1.3.0 | |
| */ | |
| signal?: AbortSignal; |
src/federation/send.ts
Outdated
| request = await signRequest( | ||
| request, | ||
| rsaKey.privateKey, | ||
| rsaKey.keyId, | ||
| { tracerProvider }, | ||
| ); |
There was a problem hiding this comment.
Implement request cancellation in fetch call
The signRequest function now receives the tracerProvider, but the main PR objective of implementing request cancellation is not addressed. The fetch call needs to be updated to support AbortSignal.
Here's how to implement request cancellation:
export async function sendActivity(
{
activity,
activityId,
keys,
inbox,
headers,
tracerProvider,
+ signal,
}: SendActivityParameters,
): Promise<void> {
// ... existing code ...
let request = new Request(inbox, {
method: "POST",
headers,
body: JSON.stringify(activity),
+ signal, // Add signal to the request
});
// ... signing code ...
try {
- response = await fetch(request);
+ response = await fetch(request, { signal }); // Pass signal to fetch
} catch (error) {
+ if (error.name === 'AbortError') {
+ logger.warn(
+ "Request to send activity {activityId} to {inbox} was aborted",
+ {
+ activityId,
+ inbox: inbox.href,
+ },
+ );
+ }
logger.error(
"Failed to send activity {activityId} to {inbox}:\n{error}",
{
activityId,
inbox: inbox.href,
error,
},
);
throw error;
}Committable suggestion skipped: line range outside the PR's diff.
| - Added options related to `AbortController`. | ||
| [[#51] [#187] by PDJ] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the changelog entry for AbortController changes.
The current entry is too brief. Consider expanding it to match the detail level of other entries by:
- Listing the specific interfaces that received the signal parameter (e.g.,
GetDocumentLoaderOptions,GetActorHandleOptions, etc.) - Explaining that this feature enables request timeout control
- Following the established format of other entries with proper indentation
Apply this diff to enhance the changelog entry:
- - Added options related to `AbortController`.
- [[#51] [#187] by PDJ]
+ - Added support for request cancellation using `AbortController`.
+ [[#51], [#187] by PDJ]
+
+ - Added optional `signal` parameter to the following interfaces:
+ - `GetDocumentLoaderOptions`
+ - `GetActorHandleOptions`
+ - `LookupObjectOptions`
+ - `LookupWebFingerOptions`
+
+ - The `signal` parameter enables control over the maximum duration
+ of outgoing API requests, preventing them from hanging indefinitely.Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Markdownlint (0.35.0)
88-88: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
src/sig/http.ts
Outdated
| if (span.isRecording()) { | ||
| span.setAttribute("http_signatures.algorithm", "rsa-sha256"); | ||
| span.setAttribute("http_signatures.signature", encodeHex(signature)); | ||
| } |
There was a problem hiding this comment.
Avoid recording sensitive 'signature' value in span attributes
Recording the signature value in span attributes may expose sensitive cryptographic material. Consider omitting this attribute or masking the value before setting it in the span to prevent potential security risks.
Apply this diff to remove the signature attribute:
if (span.isRecording()) {
span.setAttribute("http_signatures.algorithm", "rsa-sha256");
- span.setAttribute("http_signatures.signature", encodeHex(signature));
}📝 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.
| if (span.isRecording()) { | |
| span.setAttribute("http_signatures.algorithm", "rsa-sha256"); | |
| span.setAttribute("http_signatures.signature", encodeHex(signature)); | |
| } | |
| if (span.isRecording()) { | |
| span.setAttribute("http_signatures.algorithm", "rsa-sha256"); | |
| } |
src/sig/http.ts
Outdated
| if (span.isRecording()) { | ||
| span.setAttribute(`http_signatures.digest.${algo}`, encodeHex(digest)); | ||
| } |
There was a problem hiding this comment.
Avoid recording sensitive 'digest' values in span attributes
Recording the digest value may expose sensitive information. Consider omitting or masking this attribute before setting it in the span to prevent potential security risks.
Apply this diff:
if (span.isRecording()) {
- span.setAttribute(`http_signatures.digest.${algo}`, encodeHex(digest));
}📝 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.
| if (span.isRecording()) { | |
| span.setAttribute(`http_signatures.digest.${algo}`, encodeHex(digest)); | |
| } | |
| if (span.isRecording()) { | |
| } |
src/sig/http.ts
Outdated
| span?.setAttribute("http_signatures.key_id", keyId); | ||
| span?.setAttribute("http_signatures.signature", signature); | ||
| if ("algorithm" in sigValues) { | ||
| span?.setAttribute("http_signatures.algorithm", sigValues.algorithm); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary optional chaining and avoid recording sensitive 'signature' value
Since span is a required parameter of type Span, the optional chaining (span?.) is unnecessary. Also, recording the signature value from the Signature header may expose sensitive information. Consider removing the optional chaining and omitting the signature attribute.
Apply this diff:
- span?.setAttribute("http_signatures.key_id", keyId);
- span?.setAttribute("http_signatures.signature", signature);
- if ("algorithm" in sigValues) {
- span?.setAttribute("http_signatures.algorithm", sigValues.algorithm);
- }
+ if (span.isRecording()) {
+ span.setAttribute("http_signatures.key_id", keyId);
+ if ("algorithm" in sigValues) {
+ span.setAttribute("http_signatures.algorithm", sigValues.algorithm);
+ }
+ }📝 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.
| span?.setAttribute("http_signatures.key_id", keyId); | |
| span?.setAttribute("http_signatures.signature", signature); | |
| if ("algorithm" in sigValues) { | |
| span?.setAttribute("http_signatures.algorithm", sigValues.algorithm); | |
| } | |
| if (span.isRecording()) { | |
| span.setAttribute("http_signatures.key_id", keyId); | |
| if ("algorithm" in sigValues) { | |
| span.setAttribute("http_signatures.algorithm", sigValues.algorithm); | |
| } | |
| } |
src/sig/ld.ts
Outdated
| span.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| message: String(error), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Error Messages in Span Status
When setting the message in span.setStatus, use error.message to provide detailed error information. Using String(error) may not capture the error message effectively if error is an Error object.
Apply this diff in both signJsonLd and verifyJsonLd functions:
span.setStatus({
code: SpanStatusCode.ERROR,
- message: String(error),
+ message: error instanceof Error ? error.message : String(error),
});Also applies to: 439-442
src/sig/ld.ts
Outdated
| if (span.isRecording()) { | ||
| span.setAttribute("ld_signatures.type", signature.type); | ||
| span.setAttribute( | ||
| "ld_signatures.signature", | ||
| encodeHex(decodeBase64(signature.signatureValue)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid Recording Sensitive Data in Tracing Attributes
Recording the signature value as a tracing attribute (ld_signatures.signature) may expose sensitive cryptographic information. To prevent potential security risks, consider omitting this attribute or masking sensitive parts before recording.
Apply this diff in signJsonLd:
if (span.isRecording()) {
span.setAttribute("ld_signatures.type", signature.type);
- span.setAttribute(
- "ld_signatures.signature",
- encodeHex(decodeBase64(signature.signatureValue)),
- );
}
}Apply this diff in verifyJsonLd:
if (
"signatureValue" in jsonLd.signature &&
typeof jsonLd.signature.signatureValue === "string"
) {
- span.setAttribute(
- "ld_signatures.signature",
- jsonLd.signature.signatureValue,
- );
}Also applies to: 404-408
[ci skip]
[ci skip]
Related to: dahlia#173
[ci skip]
Related to: dahlia#173
Related to: dahlia#173
[ci skip]
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/webfinger/lookup.ts (2)
115-115: Consider enhancing abort handling.While the signal implementation is correct, consider handling these edge cases:
- Pre-aborted signals
- Specific error handling for aborted requests
Here's a suggested improvement:
+ // Check if already aborted + if (options?.signal?.aborted) { + throw new DOMException('Aborted', 'AbortError'); + } response = await fetch(url, { signal: options?.signal, headers: { Accept: "application/jrd+json", "User-Agent": typeof options.userAgent === "string" ? options.userAgent : getUserAgent(options.userAgent), }, redirect: "manual", }); } catch (error) { + // Specific handling for abort errors + if (error instanceof DOMException && error.name === 'AbortError') { + logger.debug( + "WebFinger request was aborted", + { url: url.href }, + ); + throw error; // Re-throw abort errors + } logger.debug( "Failed to fetch WebFinger resource descriptor: {error}", { url: url.href, error },
35-39: Consider adding timeout documentation or helper.While the signal implementation enables request cancellation, the PR's objective of limiting request time would be better served by also providing timeout guidance or a helper function.
Consider adding a helper function or documentation example for timeout setup:
/** * Helper to create an AbortSignal with timeout * @param timeoutMs Timeout in milliseconds * @returns AbortSignal that automatically aborts after timeout */ function createTimeoutSignal(timeoutMs: number): AbortSignal { const controller = new AbortController(); setTimeout(() => controller.abort(), timeoutMs); return controller.signal; } // Usage example in documentation: const options: LookupWebFingerOptions = { signal: createTimeoutSignal(5000), // 5 second timeout };src/vocab/lookup.ts (1)
Line range hint
89-124: Consider adding the lookup identifier as a span attribute.While the tracing implementation is thorough, adding the lookup identifier as a span attribute would provide valuable context for debugging and monitoring.
Add this line after starting the span:
return await tracer.startActiveSpan( "activitypub.lookup_object", async (span) => { + span.setAttribute("activitypub.lookup.identifier", identifier.toString()); try {src/vocab/actor.ts (1)
233-239: Consider optimizing abort handling for alias checks.While the signal propagation is correct, consider checking the abort signal state before processing aliases to ensure immediate abort handling:
async function verifyCrossOriginActorHandle( actorId: string, alias: string, userAgent: GetUserAgentOptions | string | undefined, tracerProvider: TracerProvider | undefined, signal?: AbortSignal | null, ): Promise<boolean> { const response = await lookupWebFinger(alias, { userAgent, tracerProvider, signal, }); if (response == null) return false; + if (signal?.aborted) return false; for (const alias of response.aliases ?? []) { if (new URL(alias).href === actorId) return true; } return false; }docs/manual/opentelemetry.md (3)
133-137: Document AbortController functionality for instrumented spansWhile the new spans are well documented, consider adding information about the abort/timeout capabilities introduced in this PR. This would help developers understand how to use the AbortController with these operations.
Add a note about timeout handling, for example:
| Operation | Span type | Description | |----------------------|-----------|-----------------------------------| - | `Federation.fetch()` | Server | Serves the incoming HTTP request. | + | `Federation.fetch()` | Server | Serves the incoming HTTP request. Supports request cancellation via AbortSignal. | - | `lookupWebFinger()` | Client | Looks up the WebFinger resource. | + | `lookupWebFinger()` | Client | Looks up the WebFinger resource. Supports timeout via AbortSignal. |
191-191: Fix grammar in attribute descriptionThere's a grammatical error in the description.
- | `activitypub.object.in_reply_to` | string[] | The URI(s) of the original object to which the object reply. | `["https://example.com/object/1"]` | + | `activitypub.object.in_reply_to` | string[] | The URI(s) of the original object to which the object replies. | `["https://example.com/object/1"]` |🧰 Tools
🪛 LanguageTool
[grammar] ~191-~191: Possible agreement error - use third-person verb forms for singular and mass nouns.
Context: ...the original object to which the object reply. | `["https://exampl...(MASS_AGREEMENT)
180-180: Add blank lines around tableAccording to Markdown best practices, tables should be surrounded by blank lines for better readability.
Add a blank line before and after the table.
🧰 Tools
🪛 Markdownlint (0.35.0)
180-180: null
Tables should be surrounded by blank lines(MD058, blanks-around-tables)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
CHANGES.md(1 hunks)docs/manual/opentelemetry.md(2 hunks)src/runtime/docloader.ts(3 hunks)src/vocab/actor.ts(4 hunks)src/vocab/lookup.ts(2 hunks)src/webfinger/lookup.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/docloader.ts
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGES.md
88-88: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
docs/manual/opentelemetry.md
180-180: null
Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 LanguageTool
docs/manual/opentelemetry.md
[grammar] ~191-~191: Possible agreement error - use third-person verb forms for singular and mass nouns.
Context: ...the original object to which the object reply. | `["https://exampl...
(MASS_AGREEMENT)
🔇 Additional comments (6)
src/webfinger/lookup.ts (1)
35-39: LGTM! Interface changes look good.
The signal property is correctly typed as AbortSignal | null and includes proper JSDoc documentation.
src/vocab/lookup.ts (2)
48-53: LGTM! Proper implementation of AbortSignal.
The signal property is correctly typed as AbortSignal | null and well-documented with JSDoc comments.
156-156: Verify abort signal handling in dependent functions.
Let's verify that both documentLoader and lookupWebFinger properly implement the abort signal handling.
✅ Verification successful
Based on the search results, I can see that:
- The
lookupWebFingerfunction insrc/webfinger/lookup.tsproperly implements signal handling:
response = await fetch(url, {
signal: options?.signal,
headers: {- The document loader in
src/runtime/docloader.tsshows multiple fetch calls but doesn't appear to forward the abort signal. This could be problematic as these operations should be cancellable.
Let's verify the document loader implementation more specifically:
Let me try a different pattern to locate the document loader implementation:
Let me try one more specific search to check the document loader implementation:
Based on the code review, I can now provide a final response:
Abort signal handling is properly implemented across the codebase
The abort signal is correctly propagated through the relevant functions:
lookupWebFingerproperly forwards the signal to its fetch callgetDocumentLoaderaccepts and forwards the signal to its fetch operations- The signal is used consistently in fetch calls across the codebase
The implementation follows best practices for cancellation handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if documentLoader and lookupWebFinger properly implement abort signal handling
# Check documentLoader implementation
echo "Checking documentLoader implementation..."
ast-grep --pattern 'function getDocumentLoader($_) {
$$$
}'
# Check lookupWebFinger implementation
echo "Checking lookupWebFinger implementation..."
ast-grep --pattern 'function lookupWebFinger($_) {
$$$
}'
# Check for fetch calls with signal
echo "Checking fetch calls with signal..."
rg -A 2 'fetch\('
Length of output: 16673
Script:
#!/bin/bash
# Get the full implementation of getDocumentLoader
ast-grep --pattern 'export function getDocumentLoader($_) {
$$$
}'
# Get the full implementation of documentLoader
ast-grep --pattern 'const documentLoader = async ($_, $_) {
$$$
}'
Length of output: 141
Script:
#!/bin/bash
# Search for document loader implementation with more context
rg -A 20 'documentLoader.*=' src/
# Also check for any fetch calls in the document loader file
rg -A 5 'fetch\(' src/runtime/docloader.ts
Length of output: 143553
Script:
#!/bin/bash
# Check document loader implementation in runtime/docloader.ts
cat src/runtime/docloader.ts
Length of output: 16535
src/vocab/actor.ts (2)
104-109: LGTM! Signal property implementation looks good.
The signal property is correctly typed as AbortSignal | null and well-documented with JSDoc comments.
175-188: LGTM! Proper error handling for AbortError.
The implementation correctly handles request abortion with proper error context preservation.
docs/manual/opentelemetry.md (1)
180-196: LGTM! Well-documented semantic attributes
The semantic attributes table is comprehensive and provides clear examples for each attribute.
🧰 Tools
🪛 LanguageTool
[grammar] ~191-~191: Possible agreement error - use third-person verb forms for singular and mass nouns.
Context: ...the original object to which the object reply. | `["https://exampl...
(MASS_AGREEMENT)
🪛 Markdownlint (0.35.0)
180-180: null
Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
| @@ -147,6 +153,7 @@ async function lookupObjectInternal( | |||
| const jrd = await lookupWebFinger(identifier, { | |||
| userAgent: options.userAgent, | |||
| tracerProvider: options.tracerProvider, | |||
| signal: options.signal, | |||
There was a problem hiding this comment.
Propagate abort signal to documentLoader calls.
While the signal is correctly passed to lookupWebFinger, it should also be propagated to the documentLoader calls to ensure consistent cancellation behavior throughout the lookup process.
Consider updating the documentLoader calls:
if (identifier.protocol === "http:" || identifier.protocol === "https:") {
try {
- const remoteDoc = await documentLoader(identifier.href);
+ const remoteDoc = await documentLoader(identifier.href, { signal: options.signal });
document = remoteDoc.document;
} catch (error) {
logger.debug("Failed to fetch remote document:\n{error}", { error });
}
}And:
try {
- const remoteDoc = await documentLoader(l.href);
+ const remoteDoc = await documentLoader(l.href, { signal: options.signal });
document = remoteDoc.document;
break;
} catch (error) {Committable suggestion skipped: line range outside the PR's diff.
The issue number that the pull request is related to.
Utilize
AbortController(Request.signal) for APIs that make outgoing requests #51The description of the change.
add AbortController
The reason why the change is needed.
to limit the maximum amount of time
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation