feat(#272): implement Azure Functions queue trigger handler registration#289
Open
nnoce14 wants to merge 19 commits into
Open
feat(#272): implement Azure Functions queue trigger handler registration#289nnoce14 wants to merge 19 commits into
nnoce14 wants to merge 19 commits into
Conversation
- Add registerAzureFunctionQueueHandler() to Cellix class for Storage Queue triggers - Add applicationServicesFactory.forSystem() for system-scoped passport (no user request context) - Create @ocom/handler-queue-community-update package with queue trigger handler - Add community-update queue schema to @ocom/service-queue-storage - Wire @ocom/handler-queue-community-update into @apps/api Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Reviewer's GuideAdds first-class Azure Storage Queue trigger support to the Cellix Azure Functions integration and wires up a new community-update queue handler end-to-end, including system-scoped application services, queue schema/registry, logging utilities, and tests/docs updates. Sequence diagram for community-update queue message processingsequenceDiagram
participant Queue as AzureStorageQueue
participant AzureFn as AzureFunctionsHost
participant Cellix as Cellix
participant QueueHandler as communityUpdateQueueHandler
participant QueueSvc as ServiceQueueStorage
participant AppFactory as ApplicationServicesFactory
participant CommunityApp as Community.updateSettings
Queue->>AzureFn: Enqueue CommunityUpdatePayload
AzureFn->>Cellix: app.storageQueue(communityUpdateQueueName, handler)
Note over Cellix,QueueHandler: handler wrapped via deferHandlerCreation
AzureFn->>QueueHandler: invoke(queueEntry, context)
QueueHandler->>QueueSvc: receiveFromCommunityUpdateQueue(queueEntry, extractQueueTriggerMetadata(context.triggerMetadata))
QueueSvc-->>QueueHandler: { id, payload }
QueueHandler->>AppFactory: forSystem({ canManageCommunitySettings: true })
AppFactory-->>QueueHandler: appServices
QueueHandler->>CommunityApp: updateSettings({ id: communityId, ...partialFields })
CommunityApp-->>QueueHandler: void
QueueHandler-->>AzureFn: complete
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
Cellix.registerAzureFunctionQueueHandler, consider makingPendingQueueHandlergeneric instead of castingoptionsandhandlerCreatortounknownso the queue handler types are preserved end-to-end and you avoid unsafe type assertions. - The
communityUpdateQueueHandlerCreatorerror handling relies onerr.message.toLowerCase().includes('community not found'); this is brittle—prefer using a well-defined error type or code fromupdateSettingsto distinguish expected 'not found' cases from other failures. - The
'community-update'queue name is duplicated in the new handler package, queue schema, and registration inapps/api/src/index.ts; consider centralizing this identifier (e.g., exported constant from the queue schema) to avoid drift if the name ever changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Cellix.registerAzureFunctionQueueHandler`, consider making `PendingQueueHandler` generic instead of casting `options` and `handlerCreator` to `unknown` so the queue handler types are preserved end-to-end and you avoid unsafe type assertions.
- The `communityUpdateQueueHandlerCreator` error handling relies on `err.message.toLowerCase().includes('community not found')`; this is brittle—prefer using a well-defined error type or code from `updateSettings` to distinguish expected 'not found' cases from other failures.
- The `'community-update'` queue name is duplicated in the new handler package, queue schema, and registration in `apps/api/src/index.ts`; consider centralizing this identifier (e.g., exported constant from the queue schema) to avoid drift if the name ever changes.
## Individual Comments
### Comment 1
<location path="packages/ocom/handler-queue-community-update/src/handler.ts" line_range="36-39" />
<code_context>
+ const popReceipt = context.triggerMetadata?.['popReceipt'] as string | undefined;
+ // biome-ignore lint/complexity/useLiteralKeys: triggerMetadata uses an index signature type that requires bracket access
+ const dequeueCount = context.triggerMetadata?.['dequeueCount'] as number | undefined;
+ const metadata = {
+ id,
+ ...(popReceipt === undefined ? {} : { popReceipt }),
+ ...(dequeueCount === undefined ? {} : { dequeueCount }),
+ };
+ let message: Awaited<ReturnType<typeof queueService.receiveFromCommunityUpdateQueue>>;
</code_context>
<issue_to_address>
**issue (bug_risk):** Metadata object omits keys entirely when values are undefined, which may conflict with consumers expecting explicit undefined fields.
Because `metadata` only includes `popReceipt` and `dequeueCount` when they’re defined, `receiveFromCommunityUpdateQueue` may receive `{ id }` with those keys missing entirely when trigger metadata is absent. Tests and the implied contract suggest these keys should always exist, even if `undefined`. If downstream code relies on that shape, construct `metadata` as:
```ts
const metadata = {
id,
popReceipt,
dequeueCount,
};
```
or otherwise ensure the object consistently contains these keys.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…des to resolve snyk vulnerabilities
- Make PendingQueueHandler<T> generic; use any[] to store heterogeneous handlers without per-field unsafe casts - Add CommunityNotFoundError class to @ocom/application-services; replace brittle string-match in handler with instanceof check - Export communityUpdateQueueName constant from @ocom/service-queue-storage; use it in @apps/api to eliminate duplicated queue name string - Add explicit QueueTriggerMetadata type annotation on metadata object; retain conditional spread (required by exactOptionalPropertyTypes: true) - Export QueueTriggerMetadata from @ocom/service-queue-storage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Test was throwing plain Error; handler now checks instanceof CommunityNotFoundError so the mock must use the typed error class. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The manual mock was missing the new constant export, causing index.test.ts to fail when index.ts consumed communityUpdateQueueName at import time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…install conflicts in CI pnpm 11 defaults to verify-deps-before-run=install, causing each concurrent turbo task to trigger pnpm install when workspace state is stale. Parallel workers SIGINT each other, killing tasks like @ocom/service-queue-storage#gen. CI already runs 'pnpm install --frozen-lockfile' before turbo tasks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
communityUpdateQueueHandlerCreator, the catch aroundreceiveFromCommunityUpdateQueuetreats all failures as an "invalid message payload" — consider narrowing this to known validation/deserialization failures or including the trigger metadata/queueId in the log so operational issues with the queue or storage account can be distinguished from bad messages. - The new
forSystem()method onAppServicesHostis now a hard requirement (used by queue handlers); if you anticipate other kinds of system scopes in future, consider allowing optional parameters (e.g., capabilities or reason) so you don’t need further breaking changes to the host interface when new system-only operations appear.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `communityUpdateQueueHandlerCreator`, the catch around `receiveFromCommunityUpdateQueue` treats all failures as an "invalid message payload" — consider narrowing this to known validation/deserialization failures or including the trigger metadata/queueId in the log so operational issues with the queue or storage account can be distinguished from bad messages.
- The new `forSystem()` method on `AppServicesHost` is now a hard requirement (used by queue handlers); if you anticipate other kinds of system scopes in future, consider allowing optional parameters (e.g., capabilities or reason) so you don’t need further breaking changes to the host interface when new system-only operations appear.
## Individual Comments
### Comment 1
<location path="packages/ocom/handler-queue-community-update/src/handler.ts" line_range="29-38" />
<code_context>
+export const communityUpdateQueueHandlerCreator = (applicationServicesFactory: ApplicationServicesFactory, queueService: QueueStorageOperations): StorageQueueHandler<CommunityUpdatePayload> => {
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding the queue name in log messages; reuse the shared `communityUpdateQueueName` constant.
The handler logs with a hardcoded `community-update:` prefix while the queue name is defined centrally as `communityUpdateQueueName` in `@ocom/service-queue-storage`. Using `communityUpdateQueueName` for the log prefix (e.g. ``context.error(`${communityUpdateQueueName}: invalid message payload: ...`)``) will keep logs and configuration in sync and avoid drift if the queue name changes.
Suggested implementation:
```typescript
*/
import { communityUpdateQueueName } from '@ocom/service-queue-storage';
export const communityUpdateQueueHandlerCreator = (applicationServicesFactory: ApplicationServicesFactory, queueService: QueueStorageOperations): StorageQueueHandler<CommunityUpdatePayload> => {
```
```typescript
`${communityUpdateQueueName}:
```
```typescript
`${communityUpdateQueueName}:
```
1. The second and third replacements are intentionally string-fragment replacements: they are meant to update any log messages (e.g. `context.error('community-update: invalid message payload: ...')`, `context.log("community-update: ...")`) to interpolate `communityUpdateQueueName` instead of hardcoding the prefix. If your log messages differ slightly (extra spaces, different punctuation), adjust the SEARCH fragments accordingly so they match the existing strings.
2. If `@ocom/service-queue-storage` is already imported elsewhere in this file, merge the new `communityUpdateQueueName` named import into the existing import statement instead of adding a separate one.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Replace hardcoded 'community-update:' string literals with the imported communityUpdateQueueName constant so logs stay in sync with config if the queue name ever changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/ocom/handler-queue-community-update/src/handler.ts" line_range="44" />
<code_context>
+ ...(popReceipt === undefined ? {} : { popReceipt }),
+ ...(dequeueCount === undefined ? {} : { dequeueCount }),
+ };
+ let message: Awaited<ReturnType<typeof queueService.receiveFromCommunityUpdateQueue>>;
+ try {
+ message = await queueService.receiveFromCommunityUpdateQueue(queueEntry, metadata);
</code_context>
<issue_to_address>
**suggestion:** Improve error logging by including the original error object for better diagnostics.
Right now only the error message string is logged, which loses stack traces and structured fields on the original error. To aid production debugging, log both the formatted message and the error object (e.g. `context.error(`${communityUpdateQueueName}: invalid message payload`, err)` if supported), or serialize selected non-sensitive fields with `JSON.stringify`. You can use the same approach for the `CommunityNotFoundError` branch if additional context would be helpful.
Suggested implementation:
```typescript
let message: Awaited<ReturnType<typeof queueService.receiveFromCommunityUpdateQueue>>;
try {
message = await queueService.receiveFromCommunityUpdateQueue(queueEntry, metadata);
} catch (err) {
const errorMessage = err instanceof Error ? err.message : String(err);
context.error(
`${communityUpdateQueueName}: invalid message payload: ${errorMessage}`,
err,
);
return;
}
```
There is likely a second `try/catch` block around `appServices.Community.Community.updateSettings` that catches `CommunityNotFoundError` or other errors. To align with this change, update that `catch` block so that:
1. It still logs the formatted error message string (as it does today), and
2. It also passes the original error object as an additional argument to `context.error(...)` (or `context.warn(...)`/`context.info(...)` if that is what it currently uses), e.g.:
```ts
catch (err) {
const errorMessage = err instanceof Error ? err.message : String(err);
context.error(`${communityUpdateQueueName}: community update failed: ${errorMessage}`, err);
// existing handling...
}
```
Adjust the log message text and log level to match the existing logging conventions in that branch.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…gnostics Log both the formatted message string and the original error so stack traces and structured error fields are preserved in Application Insights. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@sourcery-ai review |
henry-casper
pushed a commit
that referenced
this pull request
Jul 7, 2026
…install conflicts in CI pnpm 11 defaults to verify-deps-before-run=install, causing each concurrent turbo task to trigger pnpm install when workspace state is stale. Parallel workers SIGINT each other, killing tasks like @ocom/service-queue-storage#gen. CI already runs 'pnpm install --frozen-lockfile' before turbo tasks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
communityUpdateQueueHandlerCreator, the README and code comments refer to building a "request-scoped" application services instance, but the implementation correctly usesforSystem; aligning the wording to consistently describe this as a system-scoped, non-request context would reduce confusion. - When throwing
new Error('Application not started yet')in the queue handler registration path (setupLifecycle→app.storageQueuehandler), consider including the queue function name in the message to make it easier to identify which trigger was invoked before startup. - The queue handler logging currently emits only the error message (e.g.
invalid message payloadorcommunity not found); adding key trigger metadata such asid/dequeueCountinto the logged message or structured data would make diagnosing problematic messages significantly easier in production.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `communityUpdateQueueHandlerCreator`, the README and code comments refer to building a "request-scoped" application services instance, but the implementation correctly uses `forSystem`; aligning the wording to consistently describe this as a system-scoped, non-request context would reduce confusion.
- When throwing `new Error('Application not started yet')` in the queue handler registration path (`setupLifecycle` → `app.storageQueue` handler), consider including the queue function name in the message to make it easier to identify which trigger was invoked before startup.
- The queue handler logging currently emits only the error message (e.g. `invalid message payload` or `community not found`); adding key trigger metadata such as `id`/`dequeueCount` into the logged message or structured data would make diagnosing problematic messages significantly easier in production.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The trigger metadata extraction (
id,popReceipt,dequeueCount) incommunityUpdateQueueHandlerCreatoris quite specific and will likely be reused for other queue handlers; consider moving this into a shared helper (e.g., in the queue storage service or a utility module) so future handlers don’t duplicate the same logic and conventions. - The
RequestScopedHost/AppHosttypes incellix.tsnow include a genericforSystem(permissions?: P)whileAppServicesHostin@ocom/application-servicesuses a concretePartial<Domain.PermissionsSpec>; it may be worth aligning these typings so handlers usingAppHostcan benefit from the same permission type safety rather than dealing withunknown.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The trigger metadata extraction (`id`, `popReceipt`, `dequeueCount`) in `communityUpdateQueueHandlerCreator` is quite specific and will likely be reused for other queue handlers; consider moving this into a shared helper (e.g., in the queue storage service or a utility module) so future handlers don’t duplicate the same logic and conventions.
- The `RequestScopedHost`/`AppHost` types in `cellix.ts` now include a generic `forSystem(permissions?: P)` while `AppServicesHost` in `@ocom/application-services` uses a concrete `Partial<Domain.PermissionsSpec>`; it may be worth aligning these typings so handlers using `AppHost` can benefit from the same permission type safety rather than dealing with `unknown`.
## Individual Comments
### Comment 1
<location path="apps/api/src/cellix.ts" line_range="178-187" />
<code_context>
type UninitializedServiceRegistry<ContextType = unknown, AppServices = unknown> = InfrastructureServiceRegistry<ContextType, AppServices>;
-type RequestScopedHost<S, H = unknown> = {
+type RequestScopedHost<S, H = unknown, P = unknown> = {
forRequest(rawAuthHeader?: string, hints?: H): Promise<S>;
+ /**
+ * Builds a system-scoped application services instance, e.g. for background/queue-triggered work with
+ * no request context. Callers should pass only the specific permissions their operation needs
+ * (least privilege) rather than relying on an implicit default permission set.
+ */
+ forSystem(permissions?: P): Promise<S>;
};
type AppHost<AppServices> = RequestScopedHost<AppServices, unknown>;
</code_context>
<issue_to_address>
**suggestion:** Propagate the permissions type through `AppHost` so `forSystem` calls can be strongly typed.
`RequestScopedHost` now has a `forSystem(permissions?: P)` overload, but `AppHost` is still `RequestScopedHost<AppServices, unknown>`, which fixes `P` to `unknown`. As a result, callers of `AppHost.forSystem` (e.g. queue handlers) don’t get type checking for `permissions`, even though `ApplicationServicesFactory.forSystem` expects `Partial<Domain.PermissionsSpec>`.
Propagate the permissions generic into `AppHost`, e.g. `type AppHost<AppServices, P = unknown> = RequestScopedHost<AppServices, unknown, P>;`, and update the relevant `Cellix` usages to supply the concrete permissions type. This preserves runtime behavior but aligns the static API with the permissions contract.
Suggested implementation:
```typescript
options: Omit<StorageQueueFunctionOptions<T>, 'handler'>,
handlerCreator: (
applicationServicesHost: AppHost<AppServices, Partial<Domain.PermissionsSpec>>,
infrastructureRegistry: InitializedServiceRegistry,
) => StorageQueueHandler<T>,
): AzureFunctionHandlerRegistry<ContextType, AppServices>;
/**
* Finalizes configuration and starts the application.
*
type UninitializedServiceRegistry<ContextType = unknown, AppServices = unknown> = InfrastructureServiceRegistry<ContextType, AppServices>;
type RequestScopedHost<S, H = unknown, P = unknown> = {
forRequest(rawAuthHeader?: string, hints?: H): Promise<S>;
/**
* Builds a system-scoped application services instance, e.g. for background/queue-triggered work with
* no request context. Callers should pass only the specific permissions their operation needs
* (least privilege) rather than relying on an implicit default permission set.
*/
forSystem(permissions?: P): Promise<S>;
};
type AppHost<AppServices, P = unknown> = RequestScopedHost<AppServices, unknown, P>;
handlerCreator: (
applicationServicesHost: AppHost<AppServices, Partial<Domain.PermissionsSpec>>,
infrastructureRegistry: InitializedServiceRegistry,
) => HttpHandler;
}
interface PendingQueueHandler<AppServices, T = unknown> {
name: string;
options: Omit<StorageQueueFunctionOptions<T>, 'handler'>;
handlerCreator: (
applicationServicesHost: AppHost<AppServices, Partial<Domain.PermissionsSpec>>,
infrastructureRegistry: InitializedServiceRegistry,
) => StorageQueueHandler<T>;
}
type Phase = 'infrastructure' | 'context' | 'app-services' | 'handlers' | 'started';
```
1. Ensure `Domain.PermissionsSpec` is imported in `apps/api/src/cellix.ts` (e.g. `import { Domain } from '...';`) if it is not already available in this file.
2. Review other usages of `AppHost<AppServices>` and `RequestScopedHost<AppServices, unknown>` in `cellix.ts` (and related modules) to:
- Replace `RequestScopedHost<AppServices, unknown>` with `AppHost<AppServices, P>` where appropriate.
- Thread through a concrete `P` type (such as `Partial<Domain.PermissionsSpec>`) for any code paths that call `forSystem`, so that all `forSystem` use sites benefit from strong typing on `permissions`.
3. If there are generic types or factory functions that construct `AppHost` instances, consider adding a `P` generic parameter to them and defaulting to `unknown` to preserve backwards compatibility while enabling stricter typing where desired.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…mproved type safety for configuring permissions forSystem
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The generic parameter
PonCellixandAppHostis threaded throughout but only concretely used once forPartial<Domain.PermissionsSpec>; consider constraining or defaulting it closer to that shape to avoid overly generic APIs that may hide misuses offorSystem. - In
communityUpdateQueueHandlerCreator, the trigger metadata extraction and optional property spreading pattern is quite specific; if more queue handlers will follow, consider extracting a shared helper for buildingQueueTriggerMetadatato keep behavior consistent and reduce duplication. - The queue handler registration logic in
Cellix.setupLifecyclemirrors the HTTP handler registration; consider extracting a small internal helper for handler registration to reduce repetition and make it easier to apply future changes (e.g., additional lifecycle checks) to both handler types uniformly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The generic parameter `P` on `Cellix` and `AppHost` is threaded throughout but only concretely used once for `Partial<Domain.PermissionsSpec>`; consider constraining or defaulting it closer to that shape to avoid overly generic APIs that may hide misuses of `forSystem`.
- In `communityUpdateQueueHandlerCreator`, the trigger metadata extraction and optional property spreading pattern is quite specific; if more queue handlers will follow, consider extracting a shared helper for building `QueueTriggerMetadata` to keep behavior consistent and reduce duplication.
- The queue handler registration logic in `Cellix.setupLifecycle` mirrors the HTTP handler registration; consider extracting a small internal helper for handler registration to reduce repetition and make it easier to apply future changes (e.g., additional lifecycle checks) to both handler types uniformly.
## Individual Comments
### Comment 1
<location path="packages/ocom/handler-queue-community-update/src/handler.ts" line_range="36-38" />
<code_context>
+ context.error(`${communityUpdateQueueName}: invalid message payload (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'}): ${err instanceof Error ? err.message : String(err)}`, err);
+ return;
+ }
+ const appServices = await applicationServicesFactory.forSystem({ canManageCommunitySettings: true });
+ const { communityId, name, domain, whiteLabelDomain, handle } = message.payload.eventPayload;
+ try {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Errors from `forSystem` will currently crash the function without logging.
While you handle errors from `receiveFromCommunityUpdateQueue` and `updateSettings`, any exception thrown inside `applicationServicesFactory.forSystem` (e.g., misconfiguration, passport factory issues) will escape unlogged. Please wrap the `forSystem` call in its own try/catch, log via `context.error` with relevant queue/trigger metadata, and then rethrow so operational failures are visible and traceable.
```suggestion
let appServices;
try {
appServices = await applicationServicesFactory.forSystem({ canManageCommunitySettings: true });
} catch (err) {
context.error(
`${communityUpdateQueueName}: failed to create application services (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'})`,
err,
);
throw err;
}
const { communityId, name, domain, whiteLabelDomain, handle } = message.payload.eventPayload;
try {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ers to use it for improved metadata extraction
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/ocom/handler-queue-community-update/src/handler.ts" line_range="26-35" />
<code_context>
+ context.error(`${communityUpdateQueueName}: invalid message payload (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'}): ${err instanceof Error ? err.message : String(err)}`, err);
+ return;
+ }
+ let appServices: Awaited<ReturnType<typeof applicationServicesFactory.forSystem>>;
+ try {
+ appServices = await applicationServicesFactory.forSystem({ canManageCommunitySettings: true });
+ } catch (err) {
+ context.error(`${communityUpdateQueueName}: failed to create application services (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'}): ${err instanceof Error ? err.message : String(err)}`, err);
+ throw err;
+ }
+ const { communityId, name, domain, whiteLabelDomain, handle } = message.payload.eventPayload;
+ try {
+ await appServices.Community.Community.updateSettings({
+ id: communityId,
+ ...(name === undefined ? {} : { name }),
</code_context>
<issue_to_address>
**suggestion:** Consider logging metadata for non-`CommunityNotFoundError` failures before rethrowing to aid diagnostics.
The non-`CommunityNotFoundError` path currently rethrows without logging any queue context, even though `id` and `dequeueCount` are already available. You could add a single `context.error` before rethrowing, including these fields, for example:
```ts
} catch (err) {
if (err instanceof CommunityNotFoundError) {
context.error(
`${communityUpdateQueueName}: community not found: ${communityId} (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'})`,
err,
);
return;
}
context.error(
`${communityUpdateQueueName}: updateSettings failed for ${communityId} (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'})`,
err,
);
throw err;
}
```
This preserves existing retry behavior while improving log correlation for failed messages.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+26
to
+35
| let appServices: Awaited<ReturnType<typeof applicationServicesFactory.forSystem>>; | ||
| try { | ||
| appServices = await applicationServicesFactory.forSystem({ canManageCommunitySettings: true }); | ||
| } catch (err) { | ||
| context.error(`${communityUpdateQueueName}: failed to create application services (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'}): ${err instanceof Error ? err.message : String(err)}`, err); | ||
| throw err; | ||
| } | ||
| const { communityId, name, domain, whiteLabelDomain, handle } = message.payload.eventPayload; | ||
| try { | ||
| await appServices.Community.Community.updateSettings({ |
Contributor
There was a problem hiding this comment.
suggestion: Consider logging metadata for non-CommunityNotFoundError failures before rethrowing to aid diagnostics.
The non-CommunityNotFoundError path currently rethrows without logging any queue context, even though id and dequeueCount are already available. You could add a single context.error before rethrowing, including these fields, for example:
} catch (err) {
if (err instanceof CommunityNotFoundError) {
context.error(
`${communityUpdateQueueName}: community not found: ${communityId} (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'})`,
err,
);
return;
}
context.error(
`${communityUpdateQueueName}: updateSettings failed for ${communityId} (id=${id}, dequeueCount=${dequeueCount ?? 'unknown'})`,
err,
);
throw err;
}This preserves existing retry behavior while improving log correlation for failed messages.
henry-casper
added a commit
that referenced
this pull request
Jul 8, 2026
pnpm 11 only reads auth/registry settings from .npmrc; all other settings must live in pnpm-workspace.yaml (camelCase). The .npmrc added in 3b2105f was silently ignored, so parallel turbo tasks still spawned concurrent 'pnpm install' runs that SIGINT each other (runDepsStatusCheck in the CI stack trace). verifyDepsBeforeRun: false now actually resolves (pnpm config get confirms). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
henry-casper
added a commit
that referenced
this pull request
Jul 8, 2026
pnpm 11 only reads auth/registry settings from .npmrc; all other settings must live in pnpm-workspace.yaml (camelCase). The .npmrc added in 3b2105f was silently ignored, so parallel turbo tasks still spawned concurrent 'pnpm install' runs that SIGINT each other (runDepsStatusCheck in the CI stack trace). verifyDepsBeforeRun: false now actually resolves (pnpm config get confirms). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #272
Implements Azure Functions Storage Queue trigger handler registration in the Cellix framework, following the same patterns as HTTP handler registration.
Changes
New Package:
@ocom/handler-queue-community-update@apps/api— Cellix class (cellix.ts)registerAzureFunctionQueueHandler()mirroringregisterAzureFunctionHttpHandler()@ocom/handler-queue-community-updateas the first queue trigger@ocom/application-servicesforSystem()to theAppServicesHostinterfaceforSystem()— constructs a system passport with minimal permissions scoped to community settings management (no user/request context)@ocom/service-queue-storagecommunity-updateinbound queue schema and registry entryPackage naming convention
New package follows the
@ocom/handler-{trigger-type}-{purpose}convention established here:@ocom/handler-queue-community-update(this PR)@ocom/handler-http-graphql,@ocom/handler-http-rest(rename of existing packages)Test coverage
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
Summary by Sourcery
Add first-class support for Azure Storage Queue-triggered functions in the Cellix framework and wire up a community settings update queue handler into the API app.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: