WIP #513
WIP #513
Conversation
WalkthroughThe changes restructure the application initialization in In The GitHub Actions workflow disables the "lint", "check-yarn-lock" jobs and the "Run Tests" step by commenting them out, preventing linting, lockfile checking, and test execution during CI/CD without removing the job definitions. The ✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (1)
src/app.ts (1)
1-1096: Consider centralizing performance measurementsWhile the individual timing logs are helpful, consider implementing a more structured performance monitoring approach. You could create a utility function that handles timing measurements to avoid cluttering the main application code.
+ // At the top of the file or in a separate utility file + function measurePerformance<T>(name: string, fn: () => T | Promise<T>): Promise<T> { + console.time(name); + const result = fn(); + if (result instanceof Promise) { + return result.finally(() => console.timeEnd(name)); + } + console.timeEnd(name); + return Promise.resolve(result); + } + + // Then replace individual console.time/timeEnd calls with: + await measurePerformance('imports', async () => { + // import statements + });This would make the code cleaner while still providing the same timing information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks)src/app.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/app.ts (3)
src/knex.kvstore.ts (1)
KnexKvStore(4-60)src/db.ts (1)
client(3-25)src/mq/gcloud-pubsub-push/mq.ts (1)
GCloudPubSubPushMessageQueue(60-221)
🔇 Additional comments (9)
.github/workflows/build.yml (1)
95-96:Details
❓ Verification inconclusive
Tests step is commented out
The "Run Tests" step has been disabled in the GitHub Actions workflow while still keeping the step in the file. While this might be intentional for temporary debugging or expedited deployments, it poses a risk of deploying untested code to both staging and production environments.
Consider whether this is a temporary change or if you meant to make it permanent. If temporary, add a TODO comment explaining why it's commented out and when it should be restored. If permanent, consider updating the job name from "Build, Test and Push" to "Build and Push" to accurately reflect its functionality.
Critical: Verify Handling of Disabled Test Step
The workflow file
.github/workflows/build.yml(lines 95-96) still contains the commented-out “Run Tests” step. This poses a risk of deploying untested code. Please verify whether disabling tests is only temporary or if it’s meant to be a permanent change. If it’s temporary, add a TODO comment with the rationale and a timeline for re-enabling tests. Otherwise, if the change is permanent, update the job name (e.g., from “Build, Test and Push” to “Build and Push”) to accurately reflect its functionality.src/app.ts (8)
1-1: Performance measurement: Monitoring import timeAdded performance timing logs to measure how long imports take to load. This is useful for identifying potential bottlenecks during application startup.
Also applies to: 127-127
141-141: Performance measurement: Monitoring logging configuration timeAdded performance timing logs to measure how long the logging system takes to configure. Good for performance profiling.
Also applies to: 182-182
148-156: Improved logging format structureThe logging format has been restructured with better spacing and organization while maintaining the same functionality. The change improves code readability.
190-190: Performance measurement: Monitoring KV store initializationAdded performance timing logs to measure the KV store initialization time. This helps identify if the key-value store creation is a bottleneck during startup.
Also applies to: 192-192
196-196: Performance measurement: Monitoring message queue initializationAdded performance timing logs to measure message queue initialization time. Important for understanding startup delays when the message queue is enabled.
Also applies to: 224-224
246-246: Performance measurement: Monitoring database connection timeAdded performance timing logs to measure how long it takes to create the database connection. This is useful for identifying potential database connectivity issues during startup.
Also applies to: 248-248
250-250: Performance measurement: Monitoring federation queue start timeAdded performance timing logs to measure how long it takes to start the federation queue. This helps profile application startup time when manually starting the queue.
Also applies to: 258-258
742-747: Improved type annotation formattingReformatted the role assignment type cast for better readability with each role on its own line. This change is purely stylistic and does not affect functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
20-20: Prepend Startup Timing CommandThe new command on line 20 effectively prepends
console.time('startup');todist/app.jsto initiate a timing measure during the application's startup phase. Please verify that:
- A corresponding
console.timeEnd('startup');is executed later in the application (likely in the startup sequence) so that the timer is properly stopped.- The change does not interfere with the expected structure or execution of
dist/app.js(e.g., if any tooling or processes assume a specific header format).Consider adding an inline comment in the Dockerfile to explain the intent behind this command for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile(1 hunks)src/app.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.ts
rmgpinto
force-pushed
the
improve-boot-time
branch
from
5a13441 to
a54b83f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
20-20: Entry Point Update: Revisit Your Application Startup AssumptionsThe CMD command now points to
"src/boot.js"instead of"dist/app.js". Ensure that:
- The intended initialization and bootstrapping logic in
src/boot.jsis fully production-ready.- The new entry point properly handles any dependencies or configurations that were previously managed in the build process.
- The build step (
RUN yarn build) is still necessary if the application is now launched from source rather than from compiled output—verify that both approaches align with your deployment strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile(1 hunks)src/boot.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/boot.js
rmgpinto
force-pushed
the
improve-boot-time
branch
from
82c32ee to
63f72a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/app.ts (2)
531-537: Second declaration ofHonoContextVariablesis redundant.The same alias is already declared at module scope (line 196). Redefining it here provides no benefit and risks divergence.
Either remove these lines entirely or import the top‑level alias if necessary.
147-162: Run the formatter – Biome reports style drift in this block.Whitespace & indentation around the ternary
formatter:expression has diverged from the project’s style rules.
Runningbiome format(or the repo’s standard formatter) will auto‑fix the issue and keep CI green.🧰 Tools
🪛 GitHub Actions: CICD
[error] 147-148: Biome formatter error: File content differs from formatting output. Run formatter to fix code style issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/app.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/app.ts
[error] 320-320: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 199-199: Some exports are only types.
This export is a type.
This export is a type.
This export is a type.
This export is a type.
Using export type allows compilers to safely drop exports of types without looking for their definition.
Safe fix: Add inline type keywords.
(lint/style/useExportType)
🪛 GitHub Actions: CICD
src/app.ts
[error] 320-320: Biome linter error: Unexpected 'any' type used. Specify a different type. (rule: no-explicit-any)
[error] 199-199: Biome linter error: Some exports are only types. Use 'export type' for type-only exports to allow safe removal by compilers. (rule: use-export-type)
[error] 147-148: Biome formatter error: File content differs from formatting output. Run formatter to fix code style issues.
src/app.ts
Outdated
| * Fedify request context with app specific context data | ||
| * | ||
| * @see https://fedify.dev/manual/context | ||
| */ | ||
| type FedifyRequestContext = RequestContext<ContextData>; | ||
| type FedifyContext = Context<ContextData>; | ||
|
|
||
| console.time('db'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate local type aliases – remove to avoid name clashes.
type FedifyRequestContext and type FedifyContext are redeclared here, duplicating the top‑level definitions and confusing tooling.
Simply delete the re‑declarations; the top‑level exported types cover the need.
- /**
- * Fedify request context with app specific context data
- *
- * @see https://fedify.dev/manual/context
- */
- type FedifyRequestContext = RequestContext<ContextData>;
- type FedifyContext = Context<ContextData>;📝 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.
| * Fedify request context with app specific context data | |
| * | |
| * @see https://fedify.dev/manual/context | |
| */ | |
| type FedifyRequestContext = RequestContext<ContextData>; | |
| type FedifyContext = Context<ContextData>; | |
| console.time('db'); | |
| console.time('db'); |
src/app.ts
Outdated
| const fedify = createFederation<ContextData>({ | ||
| kv: fedifyKv, | ||
| queue, | ||
| manuallyStartQueue: process.env.MANUALLY_START_QUEUE === 'true', | ||
| skipSignatureVerification: | ||
| process.env.SKIP_SIGNATURE_VERIFICATION === 'true' && | ||
| ['development', 'testing'].includes(process.env.NODE_ENV || ''), | ||
| allowPrivateAddress: | ||
| process.env.ALLOW_PRIVATE_ADDRESS === 'true' && | ||
| ['development', 'testing'].includes(process.env.NODE_ENV || ''), | ||
| }); | ||
|
|
||
| process.exit(1); | ||
| /** | ||
| * Fedify request context with app specific context data | ||
| * | ||
| * @see https://fedify.dev/manual/context | ||
| */ | ||
| type FedifyRequestContext = RequestContext<ContextData>; | ||
| type FedifyContext = Context<ContextData>; | ||
|
|
||
| console.time('db'); | ||
| const db = await KnexKvStore.create(client, 'key_value'); | ||
| console.timeEnd('db'); | ||
|
|
There was a problem hiding this comment.
Shadowing fedify and db breaks the public exports – assign to the outer variables instead.
const fedify = … and const db = … create new block‑scoped bindings, leaving the exported fedify/db still undefined.
Replace the const declarations with simple assignments so the outer let variables are initialised.
- const fedify = createFederation<ContextData>({
+ fedify = createFederation<ContextData>({
kv: fedifyKv,
…
- const db = await KnexKvStore.create(client, 'key_value');
+ db = await KnexKvStore.create(client, 'key_value');This change also eliminates the need to return a different reference when other modules import these symbols.
📝 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.
| const fedify = createFederation<ContextData>({ | |
| kv: fedifyKv, | |
| queue, | |
| manuallyStartQueue: process.env.MANUALLY_START_QUEUE === 'true', | |
| skipSignatureVerification: | |
| process.env.SKIP_SIGNATURE_VERIFICATION === 'true' && | |
| ['development', 'testing'].includes(process.env.NODE_ENV || ''), | |
| allowPrivateAddress: | |
| process.env.ALLOW_PRIVATE_ADDRESS === 'true' && | |
| ['development', 'testing'].includes(process.env.NODE_ENV || ''), | |
| }); | |
| process.exit(1); | |
| /** | |
| * Fedify request context with app specific context data | |
| * | |
| * @see https://fedify.dev/manual/context | |
| */ | |
| type FedifyRequestContext = RequestContext<ContextData>; | |
| type FedifyContext = Context<ContextData>; | |
| console.time('db'); | |
| const db = await KnexKvStore.create(client, 'key_value'); | |
| console.timeEnd('db'); | |
| fedify = createFederation<ContextData>({ | |
| kv: fedifyKv, | |
| queue, | |
| manuallyStartQueue: process.env.MANUALLY_START_QUEUE === 'true', | |
| skipSignatureVerification: | |
| process.env.SKIP_SIGNATURE_VERIFICATION === 'true' && | |
| ['development', 'testing'].includes(process.env.NODE_ENV || ''), | |
| allowPrivateAddress: | |
| process.env.ALLOW_PRIVATE_ADDRESS === 'true' && | |
| ['development', 'testing'].includes(process.env.NODE_ENV || ''), | |
| }); | |
| /** | |
| * Fedify request context with app specific context data | |
| * | |
| * @see https://fedify.dev/manual/context | |
| */ | |
| type FedifyRequestContext = RequestContext<ContextData>; | |
| type FedifyContext = Context<ContextData>; | |
| console.time('db'); | |
| db = await KnexKvStore.create(client, 'key_value'); | |
| console.timeEnd('db'); |
src/app.ts
Outdated
| let fedify: ReturnType<typeof createFederation<ContextData>>; | ||
| type FedifyRequestContext = ReturnType<typeof createFederation<ContextData>>; | ||
| type FedifyContext = Context<ContextData>; | ||
| type HonoContextVariables = ContextData; | ||
| type AppContext = HonoContext<{ Variables: HonoContextVariables }> | ||
| let db: KvStore; | ||
| export { fedify, FedifyRequestContext, FedifyContext, db, HonoContextVariables, AppContext }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export‑only types should use export type, and the variable exports are currently undefined.
- Lint fails because type‑only symbols are exported with
exportinstead ofexport type. fedifyanddbare variables that are never assigned at module scope because they’re shadowed later (see next comment). Any module importing them now receivesundefined.
-let fedify: ReturnType<typeof createFederation<ContextData>>;
+let fedify: ReturnType<typeof createFederation<ContextData>> | undefined;
-export { fedify, FedifyRequestContext, FedifyContext, db, HonoContextVariables, AppContext };
+export { fedify, db };
+export type {
+ FedifyRequestContext,
+ FedifyContext,
+ HonoContextVariables,
+ AppContext,
+};📝 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.
| let fedify: ReturnType<typeof createFederation<ContextData>>; | |
| type FedifyRequestContext = ReturnType<typeof createFederation<ContextData>>; | |
| type FedifyContext = Context<ContextData>; | |
| type HonoContextVariables = ContextData; | |
| type AppContext = HonoContext<{ Variables: HonoContextVariables }> | |
| let db: KvStore; | |
| export { fedify, FedifyRequestContext, FedifyContext, db, HonoContextVariables, AppContext }; | |
| let fedify: ReturnType<typeof createFederation<ContextData>> | undefined; | |
| type FedifyRequestContext = ReturnType<typeof createFederation<ContextData>>; | |
| type FedifyContext = Context<ContextData>; | |
| type HonoContextVariables = ContextData; | |
| type AppContext = HonoContext<{ Variables: HonoContextVariables }> | |
| let db: KvStore; | |
| export { fedify, db }; | |
| export type { | |
| FedifyRequestContext, | |
| FedifyContext, | |
| HonoContextVariables, | |
| AppContext, | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Some exports are only types.
This export is a type.
This export is a type.
This export is a type.
This export is a type.
Using export type allows compilers to safely drop exports of types without looking for their definition.
Safe fix: Add inline type keywords.
(lint/style/useExportType)
🪛 GitHub Actions: CICD
[error] 199-199: Biome linter error: Some exports are only types. Use 'export type' for type-only exports to allow safe removal by compilers. (rule: use-export-type)
src/app.ts
Outdated
| function ensureCorrectContext<B, R>( | ||
| fn: (ctx: Context<ContextData>, b: B) => Promise<R>, | ||
| ) { | ||
| return async (ctx: Context<ContextData>, b: B) => { | ||
| const host = ctx.host; | ||
| if (!ctx.data) { | ||
| (ctx as any).data = {}; | ||
| } | ||
| if (!ctx.data.globaldb) { | ||
| ctx.data.globaldb = db; | ||
| } | ||
| if (!ctx.data.logger) { | ||
| ctx.data.logger = logging; | ||
| } | ||
| // Ensure scoped data / objects are initialised on each execution | ||
| // of this function - Fedify may reuse the context object across | ||
| // multiple executions of an inbox listener | ||
| ctx.data.db = scopeKvStore(db, ['sites', host]); | ||
|
|
||
| const inboxListener = fedify.setInboxListeners( | ||
| '/.ghost/activitypub/inbox/{handle}', | ||
| '/.ghost/activitypub/inbox', | ||
| ); | ||
| return fedifyContextFactory.registerContext(ctx, () => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the explicit any to satisfy the linter and improve type‑safety.
- if (!ctx.data) {
- (ctx as any).data = {};
- }
+ if (!ctx.data) {
+ // `data` is optional on the Fedify context – initialise it
+ (ctx as Context<ContextData> & { data: Partial<ContextData> }).data = {};
+ }This keeps strict type‑checking while avoiding the no‑explicit‑any violation reported by Biome.
🧰 Tools
🪛 Biome (1.9.4)
[error] 320-320: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🪛 GitHub Actions: CICD
[error] 320-320: Biome linter error: Unexpected 'any' type used. Specify a different type. (rule: no-explicit-any)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/app.ts (4)
247-254: 🛠️ Refactor suggestionRemove duplicate type declarations.
These type declarations are duplicating the ones already defined at module level (lines 194-195). The redundant declarations may confuse tooling and developers.
- /** - * Fedify request context with app specific context data - * - * @see https://fedify.dev/manual/context - */ - type FedifyRequestContext = RequestContext<ContextData>; - type FedifyContext = Context<ContextData>;
321-322:⚠️ Potential issueAvoid using explicit
anytype.Using
anydisables TypeScript's type checking, which can lead to runtime errors. Use a more specific type assertion instead.if (!ctx.data) { - (ctx as any).data = {}; + // `data` is optional on the Fedify context – initialize it + (ctx as Context<ContextData> & { data: Partial<ContextData> }).data = {}; }🧰 Tools
🪛 Biome (1.9.4)
[error] 321-321: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
193-199:⚠️ Potential issueFix export of types to satisfy the linter and improve bundling.
The types
FedifyRequestContext,FedifyContext,HonoContextVariables, andAppContextshould be exported usingexport typeto help compilers safely remove them during transpilation, as identified by the Biome linter.let fedify: ReturnType<typeof createFederation<ContextData>>; type FedifyRequestContext = ReturnType<typeof createFederation<ContextData>>; type FedifyContext = Context<ContextData>; type HonoContextVariables = ContextData; type AppContext = HonoContext<{ Variables: HonoContextVariables }> let db: KvStore; -export { fedify, FedifyRequestContext, FedifyContext, db, HonoContextVariables, AppContext }; +export { fedify, db }; +export type { FedifyRequestContext, FedifyContext, HonoContextVariables, AppContext };🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Some exports are only types.
This export is a type.
This export is a type.
This export is a type.
This export is a type.
Using export type allows compilers to safely drop exports of types without looking for their definition.
Safe fix: Add inline type keywords.(lint/style/useExportType)
235-257:⚠️ Potential issueVariables
fedifyanddbare shadowed – assign to outer variables instead.The exported module-level variables
fedifyanddbwill remainundefinedbecause you're declaring new local variables with the same names inside the async IIFE. You should assign directly to the module-level variables.- const fedify = createFederation<ContextData>({ + fedify = createFederation<ContextData>({ kv: fedifyKv, queue, manuallyStartQueue: process.env.MANUALLY_START_QUEUE === 'true', skipSignatureVerification: process.env.SKIP_SIGNATURE_VERIFICATION === 'true' && ['development', 'testing'].includes(process.env.NODE_ENV || ''), allowPrivateAddress: process.env.ALLOW_PRIVATE_ADDRESS === 'true' && ['development', 'testing'].includes(process.env.NODE_ENV || ''), }); // ... - const db = await KnexKvStore.create(client, 'key_value'); + db = await KnexKvStore.create(client, 'key_value');
🧹 Nitpick comments (2)
src/app.ts (2)
1101-1103: Remove commented-out code.The commented-out
process.exit(0)line should be removed as it's not providing any value and could confuse readers.console.timeEnd('startup'); -//process.exit(0)
673-691: Consider adding timeout or circuit breaker to key fetching.The
getKeyfunction has retry logic but could be improved with a timeout to avoid hanging indefinitely if the service is down but still responding.async function getKey(jwksURL: URL, retries = 5) { try { - const jwksResponse = await fetch(jwksURL, { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 3000); + const jwksResponse = await fetch(jwksURL, { redirect: 'follow', + signal: controller.signal }); + clearTimeout(timeoutId); const jwks = await jwksResponse.json(); const key = await jose.JWK.asKey(jwks.keys[0]); return key; } catch (err) { if (retries === 0) { throw err; } await sleep(100); return getKey(jwksURL, retries - 1); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/app.ts (12)
src/mq/gcloud-pubsub-push/mq.ts (2)
GCloudPubSubPushMessageQueue(60-221)createPushMessageHandler(249-305)src/db.ts (1)
client(3-25)src/kv-helpers.ts (1)
scopeKvStore(8-22)src/activitypub/fedify-context.factory.ts (1)
FedifyContextFactory(4-24)src/activitypub/followers.service.ts (1)
FollowersService(6-47)src/helpers/ghost.ts (1)
getSiteSettings(11-24)src/instrumentation.ts (1)
spanWrapper(60-72)src/dispatchers.ts (2)
actorDispatcher(38-74)keypairDispatcher(76-115)src/helpers/context-header.ts (1)
getTraceContext(1-15)src/helpers/request-data.ts (1)
getRequestData(8-14)src/handlers.ts (9)
getSiteDataHandler(668-700)inboxHandler(702-778)createFollowActionHandler(582-666)createUnfollowActionHandler(494-581)createLikeAction(171-295)createUnlikeAction(37-169)createReplyActionHandler(301-492)createRepostActionHandler(780-883)createDerepostActionHandler(885-1024)src/http/api/webhook.ts (2)
createPostPublishedWebhookHandler(47-108)handleWebhookSiteChanged(115-131)
🪛 Biome (1.9.4)
src/app.ts
[error] 321-321: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 199-199: Some exports are only types.
This export is a type.
This export is a type.
This export is a type.
This export is a type.
Using export type allows compilers to safely drop exports of types without looking for their definition.
Safe fix: Add inline type keywords.
(lint/style/useExportType)
🔇 Additional comments (2)
src/app.ts (2)
201-231: LGTM! Good error handling for message queue initialization.The try-catch block for message queue initialization with proper error reporting to Sentry and process exit on failure is a good practice for handling critical initialization failures.
141-142: LGTM! Good use of console timing for performance monitoring.The addition of console time measurements around key initialization phases helps track performance bottlenecks during startup. This is especially valuable for diagnosing slow startups in production environments.
Also applies to: 201-202, 232-234, 255-258, 259-268, 308-309, 1101-1101
src/app.ts
Outdated
| console.time('configure'); | ||
| (async () => { | ||
| await configure({ | ||
| contextLocalStorage: new AsyncLocalStorage(), | ||
| sinks: { | ||
| console: getConsoleSink({ | ||
| formatter: process.env.K_SERVICE | ||
| ? (record: LogRecord) => { | ||
| const loggingObject = { | ||
| timestamp: new Date(record.timestamp).toISOString(), | ||
| severity: record.level.toUpperCase(), | ||
| message: record.message.join(''), | ||
| ...record.properties, | ||
| }; | ||
|
|
||
| return JSON.stringify(loggingObject); | ||
| } | ||
| : getAnsiColorFormatter({ | ||
| timestamp: 'time', | ||
| }), | ||
| }), | ||
| }, | ||
| ], | ||
| }); | ||
| filters: {}, | ||
| loggers: [ | ||
| { | ||
| category: 'activitypub', | ||
| sinks: ['console'], | ||
| level: | ||
| toLogLevel(process.env.LOG_LEVEL_ACTIVITYPUB) || | ||
| toLogLevel(process.env.LOG_LEVEL) || | ||
| 'info', | ||
| }, | ||
| { | ||
| category: 'fedify', | ||
| sinks: ['console'], | ||
| level: | ||
| toLogLevel(process.env.LOG_LEVEL_FEDIFY) || | ||
| toLogLevel(process.env.LOG_LEVEL) || | ||
| 'warning', | ||
| }, | ||
| ], | ||
| }); | ||
| })(); | ||
| console.timeEnd('configure'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a more reliable approach for async configuration.
The logger configuration is using an immediately invoked async function expression nested inside the main IIFE. This pattern makes it difficult to ensure the configuration is completed before the rest of the initialization occurs.
Consider restructuring to ensure proper sequencing:
console.time('configure');
-(async () => {
- await configure({
+// Configure logging at the beginning of the main async IIFE
+const configureLogging = async () => {
+ return configure({
contextLocalStorage: new AsyncLocalStorage(),
sinks: {
// ...
},
filters: {},
loggers: [
// ...
],
});
-})();
+};
+
+await configureLogging();
console.timeEnd('configure');📝 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.
| console.time('configure'); | |
| (async () => { | |
| await configure({ | |
| contextLocalStorage: new AsyncLocalStorage(), | |
| sinks: { | |
| console: getConsoleSink({ | |
| formatter: process.env.K_SERVICE | |
| ? (record: LogRecord) => { | |
| const loggingObject = { | |
| timestamp: new Date(record.timestamp).toISOString(), | |
| severity: record.level.toUpperCase(), | |
| message: record.message.join(''), | |
| ...record.properties, | |
| }; | |
| return JSON.stringify(loggingObject); | |
| } | |
| : getAnsiColorFormatter({ | |
| timestamp: 'time', | |
| }), | |
| }), | |
| }, | |
| ], | |
| }); | |
| filters: {}, | |
| loggers: [ | |
| { | |
| category: 'activitypub', | |
| sinks: ['console'], | |
| level: | |
| toLogLevel(process.env.LOG_LEVEL_ACTIVITYPUB) || | |
| toLogLevel(process.env.LOG_LEVEL) || | |
| 'info', | |
| }, | |
| { | |
| category: 'fedify', | |
| sinks: ['console'], | |
| level: | |
| toLogLevel(process.env.LOG_LEVEL_FEDIFY) || | |
| toLogLevel(process.env.LOG_LEVEL) || | |
| 'warning', | |
| }, | |
| ], | |
| }); | |
| })(); | |
| console.timeEnd('configure'); | |
| console.time('configure'); | |
| // Configure logging at the beginning of the main async IIFE | |
| const configureLogging = async () => { | |
| return configure({ | |
| contextLocalStorage: new AsyncLocalStorage(), | |
| sinks: { | |
| console: getConsoleSink({ | |
| formatter: process.env.K_SERVICE | |
| ? (record: LogRecord) => { | |
| const loggingObject = { | |
| timestamp: new Date(record.timestamp).toISOString(), | |
| severity: record.level.toUpperCase(), | |
| message: record.message.join(''), | |
| ...record.properties, | |
| }; | |
| return JSON.stringify(loggingObject); | |
| } | |
| : getAnsiColorFormatter({ | |
| timestamp: 'time', | |
| }), | |
| }), | |
| }, | |
| filters: {}, | |
| loggers: [ | |
| { | |
| category: 'activitypub', | |
| sinks: ['console'], | |
| level: | |
| toLogLevel(process.env.LOG_LEVEL_ACTIVITYPUB) || | |
| toLogLevel(process.env.LOG_LEVEL) || | |
| 'info', | |
| }, | |
| { | |
| category: 'fedify', | |
| sinks: ['console'], | |
| level: | |
| toLogLevel(process.env.LOG_LEVEL_FEDIFY) || | |
| toLogLevel(process.env.LOG_LEVEL) || | |
| 'warning', | |
| }, | |
| ], | |
| }); | |
| }; | |
| await configureLogging(); | |
| console.timeEnd('configure'); |
src/app.ts
Outdated
| spanWrapper( | ||
| createRepostActionHandler( | ||
| accountRepository, | ||
| postService, | ||
| postRepository, | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove double spanWrapper wrapping.
The createRepostActionHandler is being wrapped twice with spanWrapper, which will create nested spans unnecessarily.
app.post(
'/.ghost/activitypub/actions/repost/:id',
requireRole(GhostRole.Owner, GhostRole.Administrator),
spanWrapper(
- spanWrapper(
createRepostActionHandler(
accountRepository,
postService,
postRepository,
),
- ),
),
);📝 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.
| spanWrapper( | |
| createRepostActionHandler( | |
| accountRepository, | |
| postService, | |
| postRepository, | |
| ), | |
| ), | |
| ), | |
| app.post( | |
| '/.ghost/activitypub/actions/repost/:id', | |
| requireRole(GhostRole.Owner, GhostRole.Administrator), | |
| spanWrapper( | |
| createRepostActionHandler( | |
| accountRepository, | |
| postService, | |
| postRepository, | |
| ), | |
| ), | |
| ); |
rmgpinto
force-pushed
the
improve-boot-time
branch
from
8eee2df to
4e52812
Compare
No description provided.