feat(listing): implement expired listing deletion timer (#200) #335
Conversation
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
## Summary Implements automatic deletion of expired listings after 6 months archival period, following Azure Functions timer trigger pattern. ## Changes ### Timer Handler - Added `expired-listing-deletion-handler.ts` - Azure Functions timer that runs daily at 2 AM - Integrated OpenTelemetry tracing for observability ### Application Services - Added `processExpiredDeletions` service to batch delete expired/cancelled listings older than 6 months - Added `deleteByListing` service to clean up related conversations - Added `forSystemTask()` factory method for system-level operations without user context ### Domain Layer - Added `requestDelete()` method to Conversation aggregate (following ItemListing pattern) - Added `deleteBlob()` method to BlobStorage service interface ### Persistence Layer - Added `getExpiredForDeletion()` to ItemListingReadRepository - Added `getByListingId()` to ConversationReadRepository ### Infrastructure - Added `@azure/storage-blob` to service-blob-storage for blob cleanup - Added `@sthrift/domain` dependency to context-spec ## Testing - Comprehensive unit tests for all new services - Feature files for BDD testing - Timer handler tests with OpenTelemetry mocks Closes #200
Contributor
Reviewer's GuideImplements a scheduled Azure Functions timer that runs daily to hard-delete expired or cancelled listings older than a configurable archival period, including associated conversations and listing images, by extending Cellix bootstrap, application services, domain, persistence, blob storage infrastructure, and tests. Sequence diagram for expired listing deletion timer workflowsequenceDiagram
participant Timer as AzureTimer_processExpiredListingDeletions
participant Cellix as CellixApp
participant AppHost as ApplicationServicesFactory
participant ListingSvc as ListingContextApplicationService
participant ItemListingSvc as ItemListingApplicationService
participant ReadListingRepo as ItemListingReadRepository
participant BlobStorage as BlobStorage
participant ConvSvc as ConversationApplicationService
participant ReadConvRepo as ConversationReadRepository
participant ConvUoW as ConversationUnitOfWork
participant ListingUoW as ItemListingUnitOfWork
Timer->>Cellix: trigger timer processExpiredListingDeletions
Cellix->>AppHost: get applicationServicesHost
AppHost->>AppHost: forSystemTask()
AppHost-->>ListingSvc: Listing
AppHost-->>ConvSvc: Conversation
Timer->>ItemListingSvc: processExpiredDeletions()
ItemListingSvc->>ReadListingRepo: getExpiredForDeletion(archivalMonths,batchSize)
ReadListingRepo-->>ItemListingSvc: expiredListings[]
loop for each listing in expiredListings
alt blobStorage is configured and listing has images
ItemListingSvc->>BlobStorage: deleteBlob(containerName,imagePath) * N
BlobStorage-->>ItemListingSvc: void
else no blobStorage or no images
ItemListingSvc-->>ItemListingSvc: skip image deletion
end
ItemListingSvc->>ConvSvc: deleteByListing(listingId)
ConvSvc->>ReadConvRepo: getByListingId(listingId)
ReadConvRepo-->>ConvSvc: conversations[]
loop for each conversation
ConvSvc->>ConvUoW: withScopedTransaction(callback)
ConvUoW->>ConvUoW: get(conversationId)
ConvUoW->>ConvUoW: domainConversation.requestDelete()
ConvUoW->>ConvUoW: save(domainConversation)
ConvUoW-->>ConvSvc: void
end
ItemListingSvc->>ListingUoW: withScopedTransaction(callback)
ListingUoW->>ListingUoW: get(listingId)
ListingUoW->>ListingUoW: domainListing.requestDelete()
ListingUoW->>ListingUoW: save(domainListing)
ListingUoW-->>ItemListingSvc: void
end
ItemListingSvc-->>Timer: ProcessExpiredDeletionsResult
Class diagram for application services and cleanup workflowclassDiagram
class ListingDeletionConfig {
+number archivalMonths
+number batchSize
+string blobContainerName
}
class ProcessExpiredDeletionsResult {
+number deletedCount
+string[] deletedListingIds
+number deletedConversationsCount
+number deletedImagesCount
+ErrorEntry[] errors
}
class ErrorEntry {
+string listingId
+string error
}
class DeleteByListingResult {
+number deletedCount
+string[] deletedConversationIds
+ConvErrorEntry[] errors
}
class ConvErrorEntry {
+string conversationId
+string error
}
class ItemListingApplicationService {
+create(command)
+queryById(id)
+queryByUser(userId)
+queryAll()
+cancel(command)
+update(command)
+deleteListings(command)
+unblock(command)
+queryPaged(criteria,page,pageSize)
+processExpiredDeletions() ProcessExpiredDeletionsResult
}
class ConversationApplicationService {
+create(command)
+queryById(id)
+queryByUser(userId)
+sendMessage(command)
+deleteByListing(listingId) DeleteByListingResult
}
class ListingContextApplicationService {
+ItemListing ItemListingApplicationService
}
class ConversationContextApplicationService {
+Conversation ConversationApplicationService
}
class SystemTaskServices {
+Listing ListingContextApplicationService
+Conversation ConversationContextApplicationService
}
class ApplicationServicesFactory {
+forRequest(rawAuthHeader,hints) ApplicationServices
+forSystemTask() SystemTaskServices
}
class ApplicationServices {
+Listing ListingContextApplicationService
+Conversation ConversationContextApplicationService
+ReservationRequest any
+User any
}
class DataSources {
+readonlyDataSource ReadonlyDataSource
+domainDataSource DomainDataSource
}
class ReadonlyDataSource {
+Listing ListingReadonly
+Conversation ConversationReadonly
}
class ListingReadonly {
+ItemListing ItemListingReadonly
}
class ItemListingReadonly {
+ItemListingReadRepo ItemListingReadRepository
}
class ConversationReadonly {
+Conversation ConversationReadOnlyInner
}
class ConversationReadOnlyInner {
+ConversationReadRepo ConversationReadRepository
}
class DomainDataSource {
+Listing ListingDomain
+Conversation ConversationDomain
}
class ListingDomain {
+ItemListing ItemListingDomain
}
class ItemListingDomain {
+ItemListingUnitOfWork ItemListingUnitOfWork
}
class ConversationDomain {
+Conversation ConversationDomainInner
}
class ConversationDomainInner {
+ConversationUnitOfWork ConversationUnitOfWork
}
class ItemListingReadRepository {
+getById(id)
+getByUser(userId)
+getPaged(criteria,page,pageSize)
+getExpiredForDeletion(archivalMonths,limit) ItemListingEntityReference[]
}
class ConversationReadRepository {
+getAll()
+getById(id)
+getByUser(userId)
+getBySharerReserverListing(sharerId,reserverId,listingId,options) ConversationEntityReference
+getByListingId(listingId,options) ConversationEntityReference[]
}
class Conversation {
+requestDelete() void
}
class BlobStorage {
+createValetKey(storageAccount,path,expiration) string
+deleteBlob(containerName,blobPath) void
}
class ServiceBlobStorage {
-BlobServiceClient blobServiceClient
+startUp() BlobStorage
+createValetKey(storageAccount,path,expiration) string
+deleteBlob(containerName,blobPath) void
+shutDown() void
}
ItemListingApplicationService --> ProcessExpiredDeletionsResult
ConversationApplicationService --> DeleteByListingResult
ListingContextApplicationService --> ItemListingApplicationService
ConversationContextApplicationService --> ConversationApplicationService
ApplicationServicesFactory --> ApplicationServices
ApplicationServicesFactory --> SystemTaskServices
ApplicationServices --> ListingContextApplicationService
ApplicationServices --> ConversationContextApplicationService
SystemTaskServices --> ListingContextApplicationService
SystemTaskServices --> ConversationContextApplicationService
ItemListingApplicationService --> ItemListingReadRepository
ItemListingApplicationService --> ItemListingUnitOfWork
ItemListingApplicationService --> BlobStorage
ConversationApplicationService --> ConversationReadRepository
ConversationApplicationService --> ConversationUnitOfWork
ServiceBlobStorage ..|> BlobStorage
Conversation ..> ConversationApplicationService : uses
Conversation ..> ConversationUnitOfWork : persisted via
DataSources --> ReadonlyDataSource
DataSources --> DomainDataSource
ReadonlyDataSource --> ListingReadonly
ReadonlyDataSource --> ConversationReadonly
ListingReadonly --> ItemListingReadRepository
ConversationReadonly --> ConversationReadRepository
DomainDataSource --> ListingDomain
DomainDataSource --> ConversationDomain
ListingDomain --> ItemListingUnitOfWork
ConversationDomain --> ConversationUnitOfWork
Class diagram for Cellix bootstrap and Azure Functions timer integrationclassDiagram
class RequestScopedHost~S,H~ {
+forRequest(rawAuthHeader,hints) S
}
class AppHost~AppServices~ {
+forRequest(rawAuthHeader,hints) AppServices
}
class PendingHandler~AppServices~ {
+string name
+HttpFunctionOptions options
+createHandler(applicationServicesHost) HttpHandler
}
class PendingTimerHandler~AppServices~ {
+string name
+string schedule
+createHandler(applicationServicesHost) TimerHandler
}
class AzureFunctionHandlerRegistry~ContextType,AppServices~ {
+registerAzureFunctionHttpHandler(name,options,handlerCreator) AzureFunctionHandlerRegistry
+registerAzureFunctionTimerHandler(name,schedule,handlerCreator) AzureFunctionHandlerRegistry
+startUp() StartedApplication
}
class Cellix~ContextType,AppServices~ {
-ContextType contextInternal
-RequestScopedHost~AppServices,unknown~ appServicesHostInternal
-contextCreatorInternal(serviceRegistry) ContextType
-appServicesHostBuilder(infrastructureContext) RequestScopedHost~AppServices,unknown~
-servicesInternal Map
-pendingHandlers PendingHandler~AppServices~[]
-pendingTimerHandlers PendingTimerHandler~AppServices~[]
-Phase phase
+static initializeInfrastructureServices(registerServices) ContextBuilder
+registerInfrastructureService(service) InfrastructureServiceRegistry
+setContext(contextCreator) ApplicationServicesInitializer
+initializeApplicationServices(factory) AzureFunctionHandlerRegistry
+registerAzureFunctionHttpHandler(name,options,handlerCreator) AzureFunctionHandlerRegistry
+registerAzureFunctionTimerHandler(name,schedule,handlerCreator) AzureFunctionHandlerRegistry
+startUp() StartedApplication
}
class InfrastructureServiceRegistry~ContextType,AppServices~ {
+registerInfrastructureService(service) InfrastructureServiceRegistry
}
class ApplicationServicesInitializer~ContextType,AppServices~ {
+initializeApplicationServices(factory) AzureFunctionHandlerRegistry
}
class ContextBuilder~ContextType,AppServices~ {
+setContext(contextCreator) ApplicationServicesInitializer
}
class StartedApplication~ContextType~ {
+ContextType context
}
class expiredListingDeletionHandlerCreator {
+expiredListingDeletionHandlerCreator(applicationServicesHost) TimerHandler
}
class ApplicationServicesFactory {
+forRequest(rawAuthHeader,hints) ApplicationServices
+forSystemTask() SystemTaskServices
}
class TimerHandler {
}
class HttpHandler {
}
Cellix ..|> AzureFunctionHandlerRegistry
Cellix ..|> InfrastructureServiceRegistry
Cellix ..|> ApplicationServicesInitializer
Cellix ..|> ContextBuilder
AzureFunctionHandlerRegistry --> PendingHandler
AzureFunctionHandlerRegistry --> PendingTimerHandler
Cellix --> RequestScopedHost
Cellix --> AppHost
expiredListingDeletionHandlerCreator --> TimerHandler
expiredListingDeletionHandlerCreator --> ApplicationServicesFactory
ApplicationServicesFactory ..|> AppHost
PendingHandler --> HttpHandler
PendingTimerHandler --> TimerHandler
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 3 issues, and left some high level feedback:
- In
expired-listing-deletion-handler.tsyou’re downcasting theAppServicesHost<ApplicationServices>toApplicationServicesFactoryto callforSystemTask; it would be safer to type the handler creator parameter asApplicationServicesFactory(or extend the host interface) so you don’t rely on an unchecked cast at runtime. ConversationReadRepository.getByListingIdcatches all errors and returns an empty array, which will also hide unexpected persistence issues; consider narrowing the try/catch to justObjectIdconstruction or rethrowing non-ObjectIderrors so operational problems don’t appear as “no conversations found”.process-expired-deletions.tshardcodes bothARCHIVAL_MONTHS,BATCH_SIZE, and the blob container name (listing-images); if these are likely to change per environment or over time, it may be worth routing them through configuration rather than constants to avoid future code changes for operational tuning.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `expired-listing-deletion-handler.ts` you’re downcasting the `AppServicesHost<ApplicationServices>` to `ApplicationServicesFactory` to call `forSystemTask`; it would be safer to type the handler creator parameter as `ApplicationServicesFactory` (or extend the host interface) so you don’t rely on an unchecked cast at runtime.
- `ConversationReadRepository.getByListingId` catches all errors and returns an empty array, which will also hide unexpected persistence issues; consider narrowing the try/catch to just `ObjectId` construction or rethrowing non-`ObjectId` errors so operational problems don’t appear as “no conversations found”.
- `process-expired-deletions.ts` hardcodes both `ARCHIVAL_MONTHS`, `BATCH_SIZE`, and the blob container name (`listing-images`); if these are likely to change per environment or over time, it may be worth routing them through configuration rather than constants to avoid future code changes for operational tuning.
## Individual Comments
### Comment 1
<location> `apps/api/src/timers/expired-listing-deletion-handler.ts:7-8` </location>
<code_context>
+
+const tracer = trace.getTracer('timer:expired-listing-deletion');
+
+export const expiredListingDeletionHandlerCreator = (
+ applicationServicesHost: AppServicesHost<ApplicationServices>,
+): TimerHandler => {
+ return async (timer, context) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Tighten the handler creator parameter type instead of casting to `ApplicationServicesFactory` at runtime.
This function accepts `AppServicesHost<ApplicationServices>` but immediately casts it to `ApplicationServicesFactory` to call `forSystemTask()`, which will throw if a different `AppServicesHost` is passed. Please either change the parameter (and registration) to `ApplicationServicesFactory`, or add a runtime check before casting (e.g. verifying `"forSystemTask" in applicationServicesHost`) and handle the failure path explicitly so the type matches the actual usage.
</issue_to_address>
### Comment 2
<location> `apps/api/src/cellix.ts:315-321` </location>
<code_context>
return this;
}
+ public registerAzureFunctionTimerHandler(
+ name: string,
+ schedule: string,
+ handlerCreator: (
+ applicationServicesHost: RequestScopedHost<AppServices, unknown>,
+ ) => TimerHandler,
+ ): AzureFunctionHandlerRegistry<ContextType, AppServices> {
+ this.ensurePhase('app-services', 'handlers');
+ this.pendingTimerHandlers.push({ name, schedule, handlerCreator });
</code_context>
<issue_to_address>
**issue (bug_risk):** The implementation of `registerAzureFunctionTimerHandler` uses a different `handlerCreator` type than the interface declaration.
This type mismatch means `Cellix` no longer correctly implements `AzureFunctionHandlerRegistry` and will either fail type-checking or rely on unsafe casts. Please either change the implementation to use `AppHost<AppServices>` or update the interface to the intended type so the signatures stay aligned.
</issue_to_address>
### Comment 3
<location> `packages/sthrift/persistence/src/datasources/readonly/conversation/conversation/conversation.read-repository.ts:149-158` </location>
<code_context>
}
}
+
+ async getByListingId(
+ listingId: string,
+ options?: FindOptions,
+ ): Promise<
+ Domain.Contexts.Conversation.Conversation.ConversationEntityReference[]
+ > {
+ if (!listingId || listingId.trim() === '') {
+ return [];
+ }
+
+ try {
+ const result = await this.mongoDataSource.find(
+ { listing: new MongooseSeedwork.ObjectId(listingId) },
+ { ...options, populateFields: populateFields },
+ );
+ return result.map((doc) => this.converter.toDomain(doc, this.passport));
+ } catch (error) {
+ console.warn('Error with ObjectId in getByListingId:', error);
+ return [];
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Swallowing all `getByListingId` errors and returning an empty array may hide real failures.
Because every error is caught and converted to `[]`, callers can’t tell whether there are truly no conversations or the query failed, which obscures real production issues.
Consider limiting the `try`/`catch` to just `ObjectId` creation, or rethrowing unexpected errors while only translating known `ObjectId` errors into `[]`. Another option is to return a typed error or result wrapper so callers can distinguish “not found” from “query failed.”
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ting-expired-deletion
- Remove export from ListingDependencies interface (only used internally) - Fix type safety in expired-listing-deletion-handler by using ApplicationServicesFactory directly instead of casting from AppServicesHost
Use AppHost<AppServices> type alias in both registerAzureFunctionHttpHandler and registerAzureFunctionTimerHandler implementations instead of expanding to RequestScopedHost<AppServices, unknown> for consistency with interface declarations
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The archival period, batch size, and blob container name in
process-expired-deletions.tsare hard-coded constants; consider sourcing these from configuration (env or context spec) so they can be adjusted without code changes and differ per environment. - In
ConversationReadRepository.getByListingId, all errors are caught and converted to an empty array with aconsole.warn; consider narrowing the try/catch to ObjectId construction or using the shared logging mechanism so unexpected persistence errors are not silently downgraded. - The deletion pipeline assumes all listing images reside in a single
listing-imagescontainer; if different listing types or environments may use different containers, it would be safer to pass the container name along with each image or derive it from configuration instead of hard-coding it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The archival period, batch size, and blob container name in `process-expired-deletions.ts` are hard-coded constants; consider sourcing these from configuration (env or context spec) so they can be adjusted without code changes and differ per environment.
- In `ConversationReadRepository.getByListingId`, all errors are caught and converted to an empty array with a `console.warn`; consider narrowing the try/catch to ObjectId construction or using the shared logging mechanism so unexpected persistence errors are not silently downgraded.
- The deletion pipeline assumes all listing images reside in a single `listing-images` container; if different listing types or environments may use different containers, it would be safer to pass the container name along with each image or derive it from configuration instead of hard-coding it.
## Individual Comments
### Comment 1
<location> `apps/api/src/timers/expired-listing-deletion-handler.ts:7-8` </location>
<code_context>
+
+const tracer = trace.getTracer('timer:expired-listing-deletion');
+
+export const expiredListingDeletionHandlerCreator = (
+ applicationServicesFactory: ApplicationServicesFactory,
+): TimerHandler => {
+ return async (timer, context) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** The timer handler expects an ApplicationServicesFactory, but the Cellix timer registration passes an AppHost, which likely causes a type and runtime mismatch.
In `Cellix.registerAzureFunctionTimerHandler`, `handlerCreator` is currently typed as `(applicationServicesHost: AppHost<AppServices>) => TimerHandler` and is called with `this.appServicesHostInternal`, but `expiredListingDeletionHandlerCreator` is defined as `(applicationServicesFactory: ApplicationServicesFactory) => TimerHandler`. Unless `AppHost<ApplicationServices>` exposes `forSystemTask`, this will fail type-checking and `forSystemTask` will be undefined at runtime. Please either:
- Change `registerAzureFunctionTimerHandler` to pass an `ApplicationServicesFactory`, or
- Update `expiredListingDeletionHandlerCreator` to accept an `AppHost` and obtain `forSystemTask` (or equivalent) via another mechanism.
As written, the function signatures are incompatible and will likely break at runtime.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/persistence/src/datasources/readonly/conversation/conversation/conversation.read-repository.test.ts:433` </location>
<code_context>
+ }),
+ ];
+ // Update mock model to return new conversations
+ const createMockQuery = (queryResult: unknown) => {
+ const mockQuery = {
+ lean: vi.fn(),
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared mocking helpers and centralising cleanup so the new listing scenarios focus on behaviour rather than repeated query/mocking boilerplate.
You can keep the new coverage but reduce complexity and duplication by centralising the mocking patterns and separating framework plumbing from the scenarios.
### 1. Reuse a single query-chain helper instead of inline `createMockQuery`
Right now `createMockQuery` is re‑implemented inside the `Given` for `"listing-1"`. That makes the scenario harder to scan and duplicates a tricky mocking pattern.
Extract a single helper at the top of the file (or reuse the existing `createQueryChain` if compatible) and reuse it for all `find` mocks:
```ts
// near other helpers in this file
const createFindQueryChain = <T>(queryResult: T) => {
const mockQuery = {
lean: vi.fn(),
populate: vi.fn(),
exec: vi.fn().mockResolvedValue(queryResult),
catch: vi.fn((onReject) => Promise.resolve(queryResult).catch(onReject)),
};
mockQuery.lean.mockReturnValue(mockQuery);
mockQuery.populate.mockReturnValue(mockQuery);
// biome-ignore lint/suspicious/noThenProperty: Intentional thenable mock for Mongoose queries
Object.defineProperty(mockQuery, 'then', {
value: vi.fn((onResolve) => Promise.resolve(queryResult).then(onResolve)),
enumerable: false,
configurable: true,
});
return mockQuery;
};
```
Then the scenario becomes declarative:
```ts
Given('a Conversation document with listing "listing-1"', () => {
mockConversations = [
makeMockConversation({
listing: makeMockListing('listing-1'),
}),
];
mockModel.find = vi
.fn(() => createFindQueryChain(mockConversations)) as unknown as typeof mockModel.find;
});
```
### 2. Centralise `mockModel.find` setup in a helper
Multiple scenarios mutate `mockModel.find` inline (`listing-1`, no conversations, etc.). A small helper keeps each scenario readable and avoids repeating the cast:
```ts
// near other helpers
const setFindResult = <T>(result: T) => {
mockModel.find = vi
.fn(() => createFindQueryChain(result)) as unknown as typeof mockModel.find;
};
```
Usage:
```ts
Given('a Conversation document with listing "listing-1"', () => {
mockConversations = [
makeMockConversation({
listing: makeMockListing('listing-1'),
}),
];
setFindResult(mockConversations);
});
When('I call getByListingId with "listing-without-conversations"', async () => {
setFindResult([]);
result = await repository.getByListingId(createValidObjectId('listing-without-conversations'));
});
```
This keeps the scenarios focused on domain behaviour instead of mocking details.
### 3. Move `vi.restoreAllMocks()` out of `When` steps
Mixing test cleanup into the `When` steps couples business behaviour with framework lifecycle. You can keep the same safety by moving `restoreAllMocks` to a shared `afterEach`/`afterAll`:
```ts
// inside the relevant describe/suite for this feature
afterEach(() => {
vi.restoreAllMocks();
});
```
Then remove the inline calls:
```ts
When('I call getBySharerReserverListing with invalid ID', async () => {
result = await repository.getBySharerReserverListing(
'invalid-id',
createValidObjectId('reserver'),
createValidObjectId('listing'),
);
});
When('I call getByListingId with invalid ID', async () => {
result = await repository.getByListingId('invalid-id');
});
```
This preserves all existing behaviour but makes each scenario easier to read and reason about: steps describe only conversation/listing behaviour, while mocking and cleanup are handled in small, reusable helpers.
</issue_to_address>
### Comment 3
<location> `packages/sthrift/application-services/src/contexts/listing/item/process-expired-deletions.test.ts:84` </location>
<code_context>
+ }),
+ };
+
+ mockDataSources = {
+ domainDataSource: {
+ Conversation: {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting helper factories for data sources and blob storage so each scenario builds its own mocks instead of mutating shared state in Background.
You can keep all behaviour but reduce complexity by extracting small helpers and narrowing shared mutable state. For example:
### 1. Extract a factory for data sources
Instead of rebuilding the full `mockDataSources` shape inline and mutating `mockExpiredListings`/`mockConversations`, push those into a helper that returns fresh mocks per scenario:
```ts
function createDataSources({
expiredListings = [],
conversations = [],
shouldFailListingDeletion = false,
}: {
expiredListings?: Array<{ id: string; images: string[] }>;
conversations?: Array<{ id: string }>;
shouldFailListingDeletion?: boolean;
}): {
dataSources: DataSources;
listingRepo: { get: ReturnType<typeof vi.fn>; save: ReturnType<typeof vi.fn> };
conversationRepo: { get: ReturnType<typeof vi.fn>; save: ReturnType<typeof vi.fn> };
} {
const listingRepo = {
get: vi.fn().mockImplementation(() => ({ requestDelete: vi.fn() })),
save: vi.fn().mockResolvedValue(undefined),
};
const conversationRepo = {
get: vi.fn().mockImplementation(() => ({ requestDelete: vi.fn() })),
save: vi.fn().mockResolvedValue(undefined),
};
const listingUow = {
withScopedTransaction: vi.fn((callback: (repo: typeof listingRepo) => unknown) => {
if (shouldFailListingDeletion) {
return Promise.reject(new Error('Failed to delete listing'));
}
return Promise.resolve(callback(listingRepo));
}),
};
const conversationUow = {
withScopedTransaction: vi.fn((callback: (repo: typeof conversationRepo) => unknown) =>
Promise.resolve(callback(conversationRepo)),
),
};
const dataSources = {
domainDataSource: {
Listing: {
ItemListing: {
ItemListingUnitOfWork: listingUow,
},
},
Conversation: {
Conversation: {
ConversationUnitOfWork: conversationUow,
},
},
},
readonlyDataSource: {
Listing: {
ItemListing: {
ItemListingReadRepo: {
getExpiredForDeletion: vi.fn().mockResolvedValue(expiredListings),
},
},
},
Conversation: {
Conversation: {
ConversationReadRepo: {
getByListingId: vi.fn().mockResolvedValue(conversations),
},
},
},
},
} as unknown as DataSources;
return { dataSources, listingRepo, conversationRepo };
}
```
Then scenarios configure their own data sources instead of mutating shared vars:
```ts
Scenario('Successfully deleting multiple expired listings', ({ When, Then }) => {
const expiredListings = [
{ id: 'listing-1', images: [] },
{ id: 'listing-2', images: [] },
{ id: 'listing-3', images: [] },
];
const { dataSources } = createDataSources({ expiredListings });
When('the expired deletion process runs', async () => {
const processFunction = processExpiredDeletions(dataSources, mockBlobStorage);
result = await processFunction();
});
Then('the result should show 3 deleted listings', () => {
expect(result.deletedCount).toBe(3);
});
});
```
This removes `mockExpiredListings`, `mockConversations`, `mockListingUow`, `mockConversationUow`, `shouldFailListingDeletion` from the Background and keeps the unit-of-work behaviour in one place.
### 2. Extract a blob storage mock helper
Similarly, encapsulate the call‑count / failure behaviour:
```ts
function createBlobStorageMock({
failFirstDelete = false,
}: {
failFirstDelete?: boolean;
} = {}): {
blobStorage: Domain.Services['BlobStorage'];
deleteBlobSpy: ReturnType<typeof vi.fn>;
} {
let callCount = 0;
const deleteBlobSpy = vi.fn().mockImplementation(() => {
callCount++;
if (failFirstDelete && callCount === 1) {
return Promise.reject(new Error('Blob delete failed'));
}
return Promise.resolve();
});
const blobStorage = { deleteBlob: deleteBlobSpy } as unknown as Domain.Services['BlobStorage'];
return { blobStorage, deleteBlobSpy };
}
```
Then the Background only needs high-level knobs:
```ts
Background(({ Given }) => {
Given('the blob storage service is available', () => {
({ blobStorage: mockBlobStorage, deleteBlobSpy } = createBlobStorageMock());
});
});
```
And a scenario that needs failure configures it locally instead of mutating `shouldFailBlobDeletion`:
```ts
Scenario('Handling image deletion failure gracefully', ({ When, Then }) => {
const expiredListings = [{ id: 'listing-1', images: ['image1.jpg', 'image2.jpg'] }];
const { dataSources } = createDataSources({ expiredListings });
({ blobStorage: mockBlobStorage, deleteBlobSpy } = createBlobStorageMock({ failFirstDelete: true }));
When('the expired deletion process runs', async () => {
const processFunction = processExpiredDeletions(dataSources, mockBlobStorage);
result = await processFunction();
});
Then('the result should show 1 deleted listings', () => {
expect(result.deletedCount).toBe(1);
});
});
```
### 3. Narrow Background to “environment”, not per‑scenario state
With these helpers, Background can be reduced to static setup (feature file wiring, maybe default blob storage) and any truly global state. Each scenario then:
- Builds its own `expiredListings`, `conversations` data.
- Calls `createDataSources` / `createBlobStorageMock` with explicit options.
- Avoids mutating shared arrays/flags, reducing coupling and making each scenario self-contained.
</issue_to_address>
### Comment 4
<location> `packages/sthrift/application-services/src/contexts/conversation/conversation/delete-by-listing.test.ts:36` </location>
<code_context>
+ errors: Array<{ conversationId: string; error: string }>;
+ };
+
+ Background(({ Given, And }) => {
+ Given('the data sources are available', () => {
+ shouldFailFirstConversation = false;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a reusable test harness factory so each scenario configures its own conversations and failure behavior instead of relying on shared mutable state in the Background block.
You can reduce the shared mutable complexity in the `Background` by extracting a small harness factory and letting each `Scenario` build its own configuration, instead of mutating `mockConversations` and `shouldFailFirstConversation` and relying on an internal `callCount`.
For example, pull the wiring of `mockDataSources`, `callCount`, and the failure flag into a helper:
```ts
function createDeleteByListingHarness({
conversations,
failFirstConversation = false,
}: {
conversations: Array<{ id: string }>;
failFirstConversation?: boolean;
}) {
let callCount = 0;
const mockConversationRepo = {
get: vi.fn().mockImplementation(() => ({ requestDelete: vi.fn() })),
save: vi.fn().mockResolvedValue(undefined),
};
const mockConversationUow = {
withScopedTransaction: vi.fn(async (callback: any) => {
callCount++;
if (failFirstConversation && callCount === 1) {
throw new Error('Failed to delete conversation');
}
return callback(mockConversationRepo);
}),
};
const mockDataSources = {
domainDataSource: {
Conversation: {
Conversation: {
ConversationUnitOfWork: mockConversationUow,
},
},
},
readonlyDataSource: {
Conversation: {
Conversation: {
ConversationReadRepo: {
getByListingId: vi.fn().mockResolvedValue(conversations),
},
},
},
},
} as unknown as DataSources;
return {
deleteFn: deleteByListing(mockDataSources),
mockConversationRepo,
mockConversationUow,
};
}
```
Then each scenario can express its setup explicitly without touching shared state:
```ts
Scenario('Successfully deleting a single conversation for a listing', ({ When, Then }) => {
const { deleteFn } = createDeleteByListingHarness({
conversations: [{ id: 'conv-1' }],
});
let result: Awaited<ReturnType<typeof deleteFn>>;
When('conversations are deleted for listing id {string}', async () => {
result = await deleteFn('listing-single-conv');
});
Then('the result should show 1 deleted conversations', () => {
expect(result.deletedCount).toBe(1);
expect(result.deletedConversationIds).toContain('conv-1');
});
});
```
And for the failure case:
```ts
const { deleteFn } = createDeleteByListingHarness({
conversations: [{ id: 'conv-1' }, { id: 'conv-2' }],
failFirstConversation: true,
});
```
This keeps all current behaviour (including the “fail the first transaction” semantics) but removes:
- The global `mockConversations` and `shouldFailFirstConversation` mutations.
- The need to mentally track `callCount` in each scenario.
- The large `Background` block that every scenario must understand before reading the actual test logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...ges/sthrift/application-services/src/contexts/listing/item/process-expired-deletions.test.ts
Show resolved
Hide resolved
...thrift/application-services/src/contexts/conversation/conversation/delete-by-listing.test.ts
Show resolved
Hide resolved
- Use mockResolvedValue instead of mockImplementation where appropriate - Ensure Promise.resolve wraps callback returns for consistency - Keep existing test patterns while improving readability
…uration - Add ListingDeletionConfig interface to @sthrift/context-spec for archivalMonths, batchSize, and blobContainerName - Thread configuration from environment variables through ApiContextSpec to application services - Update process-expired-deletions.ts to accept config parameter instead of hard-coded constants - Improve error handling in ConversationReadRepository: * Narrow try/catch scope to ObjectId construction only * Add structured error logging with context (listingId, userId, etc.) * Replace console.warn with console.error for better visibility - Update all tests to pass mock configuration - Add generic type parameter to registerAzureFunctionTimerHandler for flexible factory typing Resolves Sourcery feedback: archival period, batch size, and blob container now configurable per environment
…tion timer registration
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
processExpiredDeletions, all listings and their images are processed sequentially; consider parallelizing per listing (or at least image deletion viaPromise.allSettled) to keep the daily timer run performant whenbatchSizeis large. processExpiredDeletionscurrently only records listing-level failures inerrors, while conversation deletion failures are logged and discarded; if these errors are operationally relevant, consider propagating them into the result (e.g., by attaching conversation-level errors to the listing entry) so callers can see the full failure picture.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `processExpiredDeletions`, all listings and their images are processed sequentially; consider parallelizing per listing (or at least image deletion via `Promise.allSettled`) to keep the daily timer run performant when `batchSize` is large.
- `processExpiredDeletions` currently only records listing-level failures in `errors`, while conversation deletion failures are logged and discarded; if these errors are operationally relevant, consider propagating them into the result (e.g., by attaching conversation-level errors to the listing entry) so callers can see the full failure picture.
## Individual Comments
### Comment 1
<location> `packages/sthrift/persistence/src/datasources/readonly/listing/item/item-listing.read-repository.ts:200-203` </location>
<code_context>
}
}
+
+ async getExpiredForDeletion(
+ archivalMonths: number,
+ limit = 100,
+ ): Promise<Domain.Contexts.Listing.ItemListing.ItemListingEntityReference[]> {
+ const cutoffDate = new Date();
+ cutoffDate.setMonth(cutoffDate.getMonth() - archivalMonths);
</code_context>
<issue_to_address>
**issue:** Validate `archivalMonths` to avoid unexpected date calculation when misconfigured
This relies on `archivalMonths` being a positive integer, but since it comes from config/env it could be `0`, negative, or `NaN`, which would make the `setMonth` call compute an incorrect cutoff and potentially delete the wrong items. Add validation (e.g., throw or early-return when `archivalMonths <= 0` or `!Number.isFinite(archivalMonths)`).
</issue_to_address>
### Comment 2
<location> `apps/api/src/cellix.ts:121` </location>
<code_context>
+ *
+ * @throws Error - If called before application services are initialized.
+ */
+ registerAzureFunctionTimerHandler<TFactory extends RequestScopedHost<AppServices, unknown> = AppHost<AppServices>>(
+ name: string,
+ schedule: string,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the timer registration API and unifying pending handler wiring so the lifecycle and types mirror the HTTP path and are easier to follow.
You can keep the new timer feature but simplify the types and lifecycle wiring to reduce cognitive load.
### 1. Drop the unused generic + cast on `registerAzureFunctionTimerHandler`
The generic `TFactory` is immediately erased via a cast when pushing into `pendingTimerHandlers`, so it doesn’t buy type-safety but does add complexity.
You can align the timer registration API with the HTTP one and avoid the assertion:
```ts
// Before
public registerAzureFunctionTimerHandler<
TFactory extends RequestScopedHost<AppServices, unknown> = AppHost<AppServices>
>(
name: string,
schedule: string,
handlerCreator: (
applicationServicesFactory: TFactory,
) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingTimerHandlers.push({
name,
schedule,
handlerCreator: handlerCreator as (
host: RequestScopedHost<AppServices, unknown>
) => TimerHandler
});
this.phase = 'handlers';
return this;
}
```
```ts
// After – simpler, consistent with HTTP
public registerAzureFunctionTimerHandler(
name: string,
schedule: string,
handlerCreator: (
applicationServicesHost: AppHost<AppServices>,
) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingTimerHandlers.push({ name, schedule, handlerCreator });
this.phase = 'handlers';
return this;
}
```
And simplify `PendingTimerHandler` accordingly:
```ts
interface PendingTimerHandler<AppServices> {
name: string;
schedule: string;
handlerCreator: (applicationServicesHost: AppHost<AppServices>) => TimerHandler;
}
```
This keeps the same runtime behavior while making the API symmetric with HTTP and removing the need for the cast and explanatory comment.
---
### 2. Unify pending handler representation and startup wiring
You can remove the separate `PendingTimerHandler` type and `pendingTimerHandlers` list by using a discriminated union. This also lets you consolidate the duplicated registration loops in `setupLifecycle()`.
```ts
// Replace PendingHandler + PendingTimerHandler with:
type PendingEndpoint<AppServices> =
| {
kind: 'http';
name: string;
options: Omit<HttpFunctionOptions, 'handler'>;
handlerCreator: (host: AppHost<AppServices>) => HttpHandler;
}
| {
kind: 'timer';
name: string;
schedule: string;
handlerCreator: (host: AppHost<AppServices>) => TimerHandler;
};
// Single queue
private readonly pendingEndpoints: Array<PendingEndpoint<AppServices>> = [];
```
Update registration:
```ts
public registerAzureFunctionHttpHandler(
name: string,
options: Omit<HttpFunctionOptions, 'handler'>,
handlerCreator: (applicationServicesHost: AppHost<AppServices>) => HttpHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingEndpoints.push({
kind: 'http',
name,
options,
handlerCreator,
});
this.phase = 'handlers';
return this;
}
public registerAzureFunctionTimerHandler(
name: string,
schedule: string,
handlerCreator: (applicationServicesHost: AppHost<AppServices>) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingEndpoints.push({
kind: 'timer',
name,
schedule,
handlerCreator,
});
this.phase = 'handlers';
return this;
}
```
And consolidate `setupLifecycle()` wiring:
```ts
private setupLifecycle(): void {
for (const e of this.pendingEndpoints) {
if (e.kind === 'http') {
app.http(e.name, {
...e.options,
handler: (request, context) => {
if (!this.appServicesHostInternal) {
throw new Error('Application not started yet');
}
return e.handlerCreator(this.appServicesHostInternal)(request, context);
},
});
} else {
app.timer(e.name, {
schedule: e.schedule,
handler: (timer, context) => {
if (!this.appServicesHostInternal) {
throw new Error('Application not started yet');
}
return e.handlerCreator(this.appServicesHostInternal)(timer, context);
},
});
}
}
// existing app.hook.appStart(...)
}
```
This keeps all existing functionality (including phase behavior and lazy binding to `appServicesHostInternal`) while:
- Removing duplicated interfaces/queues.
- Removing the duplicated startup loops.
- Keeping the registration and lifecycle wiring easier to follow.
</issue_to_address>
### Comment 3
<location> `packages/sthrift/application-services/src/contexts/listing/item/index.ts:68` </location>
<code_context>
+ listingDeletionConfig?: ListingDeletionConfig;
}
export const ItemListing = (
- dataSources: DataSources,
+ deps: DataSources | ItemListingDependencies,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the ItemListing factory by overloading to a single dependency shape and extracting the default deletion config constant to avoid repeated narrowing and inline branching.
You can keep the new functionality and backward compatibility while reducing the runtime/type complexity by normalizing to a single dependency shape and extracting the default config.
### 1. Normalize to `ItemListingDependencies` via overloads
Instead of threading `'in'` checks throughout the implementation, use overloads and normalize once:
```ts
export interface ItemListingDependencies {
dataSources: DataSources;
blobStorage?: Domain.Services['BlobStorage'];
listingDeletionConfig?: ListingDeletionConfig;
}
export function ItemListing(
dataSources: DataSources,
): ItemListingApplicationService;
export function ItemListing(
deps: ItemListingDependencies,
): ItemListingApplicationService;
export function ItemListing(
arg: DataSources | ItemListingDependencies,
): ItemListingApplicationService {
const deps: ItemListingDependencies =
'dataSources' in arg ? arg : { dataSources: arg };
const deletionConfig = deps.listingDeletionConfig ?? DEFAULT_DELETION_CONFIG;
return {
create: create(deps.dataSources),
queryById: queryById(deps.dataSources),
queryBySharer: queryBySharer(deps.dataSources),
queryAll: queryAll(deps.dataSources),
cancel: cancel(deps.dataSources),
update: update(deps.dataSources),
deleteListings: deleteListings(deps.dataSources),
unblock: unblock(deps.dataSources),
queryPaged: queryPaged(deps.dataSources),
processExpiredDeletions: processExpiredDeletions(
deps.dataSources,
deletionConfig,
deps.blobStorage,
),
};
}
```
Callers can still pass `DataSources` directly, but the implementation only deals with `ItemListingDependencies`, which removes repeated narrowing and makes the factory easier to scan.
### 2. Extract the default deletion config
Pull the default into a constant so the control flow around config is clearer and reuse is trivial:
```ts
const DEFAULT_DELETION_CONFIG: ListingDeletionConfig = {
archivalMonths: 6,
batchSize: 100,
blobContainerName: 'listing-images',
};
```
Then in the factory (as above):
```ts
const deletionConfig =
deps.listingDeletionConfig ?? DEFAULT_DELETION_CONFIG;
```
This keeps the behavior identical (including tests/backwards compatibility), but reduces inline branching and makes the configuration story easier to understand at a glance.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...es/sthrift/persistence/src/datasources/readonly/listing/item/item-listing.read-repository.ts
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The Cellix handler APIs are now asymmetric (HTTP handlers take an AppHost while timer handlers use a generic RequestScopedHost) and
expiredListingDeletionHandlerCreatorexpects anApplicationServicesFactorywithforSystemTask; it would be clearer and less error‑prone to standardize these types so that the object passed into all handler creators consistently exposesforRequest/forSystemTask(e.g., useApplicationServicesFactoryfor timers and HTTP, or wrapAppHostappropriately). - The Mongoose query mocking helpers (
createNullPopulateChain, the inlinecreateMockQueryin conversation tests, andcreateQueryChainin listing tests) are very similar but duplicated; consider extracting a single reusable helper to reduce repetition and make future adjustments to the query shape or thenable behavior in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Cellix handler APIs are now asymmetric (HTTP handlers take an AppHost while timer handlers use a generic RequestScopedHost) and `expiredListingDeletionHandlerCreator` expects an `ApplicationServicesFactory` with `forSystemTask`; it would be clearer and less error‑prone to standardize these types so that the object passed into all handler creators consistently exposes `forRequest`/`forSystemTask` (e.g., use `ApplicationServicesFactory` for timers and HTTP, or wrap `AppHost` appropriately).
- The Mongoose query mocking helpers (`createNullPopulateChain`, the inline `createMockQuery` in conversation tests, and `createQueryChain` in listing tests) are very similar but duplicated; consider extracting a single reusable helper to reduce repetition and make future adjustments to the query shape or thenable behavior in one place.
## Individual Comments
### Comment 1
<location> `apps/api/src/cellix.ts:315-324` </location>
<code_context>
return this;
}
+ public registerAzureFunctionTimerHandler<TFactory extends RequestScopedHost<AppServices, unknown> = AppHost<AppServices>>(
+ name: string,
+ schedule: string,
+ handlerCreator: (
+ applicationServicesFactory: TFactory,
+ ) => TimerHandler,
+ ): AzureFunctionHandlerRegistry<ContextType, AppServices> {
+ this.ensurePhase('app-services', 'handlers');
+ // Type assertion is safe here because TFactory extends RequestScopedHost<AppServices, unknown>
+ // and the actual runtime value passed will be compatible with whatever TFactory the caller specified
+ this.pendingTimerHandlers.push({
+ name,
+ schedule,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The generic and cast in registerAzureFunctionTimerHandler add complexity and rely on an unsafe assumption.
`registerAzureFunctionTimerHandler` introduces `TFactory` but then casts `handlerCreator` to `(host: RequestScopedHost<...>) => TimerHandler` when storing it, and the registry is always called with `this.appServicesHostInternal`. Since only a single concrete host type is used in practice, you could either:
- Remove the generic and take `handlerCreator: (host: RequestScopedHost<AppServices, unknown>) => TimerHandler`, or
- Wrap `handlerCreator` at the call site into a `(host: RequestScopedHost<...>) => TimerHandler` instead of casting.
This removes the unsafe cast and keeps the timer registration API simpler and safer.
Suggested implementation:
```typescript
public registerAzureFunctionTimerHandler(
name: string,
schedule: string,
handlerCreator: (
applicationServicesHost: RequestScopedHost<AppServices, unknown>,
) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingTimerHandlers.push({
name,
schedule,
handlerCreator,
});
this.phase = 'handlers';
return this;
}
```
You will need to update all call sites of `registerAzureFunctionTimerHandler` to remove the generic type argument (if any) and accept a `RequestScopedHost<AppServices, unknown>` parameter in their `handlerCreator` function. If any callers relied on a more specific factory type (e.g., `AppHost<AppServices>`), they can keep their parameter typed as that specific type and rely on TypeScript’s structural typing, or explicitly annotate it as `RequestScopedHost<AppServices, unknown>`.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/application-services/src/contexts/listing/item/index.ts:68` </location>
<code_context>
+ listingDeletionConfig?: ListingDeletionConfig;
}
export const ItemListing = (
- dataSources: DataSources,
+ deps: DataSources | ItemListingDependencies,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the ItemListing factory to use overloads plus a dependency-normalization helper so the wiring logic stays simple and type-safe while preserving backward compatibility.
You can keep backward compatibility while simplifying the factory’s control flow by:
1. Using overloads instead of a union in the implementation type.
2. Centralizing dependency normalization (including defaults) into a helper.
That keeps `ItemListing` as a simple wiring function and makes types easier to follow.
Example refactor:
```ts
export interface ItemListingDependencies {
dataSources: DataSources;
blobStorage?: Domain.Services['BlobStorage'];
listingDeletionConfig?: ListingDeletionConfig;
}
const defaultDeletionConfig: ListingDeletionConfig = {
archivalMonths: 6,
batchSize: 100,
blobContainerName: 'listing-images',
};
function normalizeDeps(
deps: DataSources | ItemListingDependencies,
): ItemListingDependencies {
if ('dataSources' in deps) {
return {
...deps,
listingDeletionConfig: deps.listingDeletionConfig ?? defaultDeletionConfig,
};
}
return {
dataSources: deps,
listingDeletionConfig: defaultDeletionConfig,
};
}
// Overloads for better typing at call sites
export function ItemListing(dataSources: DataSources): ItemListingApplicationService;
export function ItemListing(deps: ItemListingDependencies): ItemListingApplicationService;
// Single implementation
export function ItemListing(
deps: DataSources | ItemListingDependencies,
): ItemListingApplicationService {
const { dataSources, blobStorage, listingDeletionConfig } = normalizeDeps(deps);
return {
create: create(dataSources),
queryById: queryById(dataSources),
queryBySharer: queryBySharer(dataSources),
queryAll: queryAll(dataSources),
cancel: cancel(dataSources),
update: update(dataSources),
deleteListings: deleteListings(dataSources),
unblock: unblock(dataSources),
queryPaged: queryPaged(dataSources),
processExpiredDeletions: processExpiredDeletions(
dataSources,
listingDeletionConfig!,
blobStorage,
),
};
}
```
This keeps:
- Both construction styles (`DataSources` only and full `ItemListingDependencies`).
- Defaulting logic in one place (`normalizeDeps`), which can be reused by the higher-level `Listing` factory to avoid repeating the union-discrimination and defaulting logic there.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…on timer registration
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
expired-listing-deletion-handler, theapplicationServicesHostis typed as a genericRequestScopedHostbut is cast toApplicationServicesFactoryto accessforSystemTask; consider tightening the Cellix/timer registration types so the handler creator receives anApplicationServicesFactory(or a host that explicitly exposesforSystemTask) and avoid the runtime cast. - The
deleteByListingapplication service runs a separatewithScopedTransactionper conversation, which can become expensive for listings with many conversations; consider wrapping all deletions for a listing in a single unit-of-work/transaction where your domain and infrastructure allow it. - The
processExpiredDeletionsloop deletes listings, conversations, and blobs fully sequentially; if you expect large batches, you might want to introduce bounded concurrency (e.g., per-listing or per-blob) to reduce total run time while still keeping per-listing error isolation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `expired-listing-deletion-handler`, the `applicationServicesHost` is typed as a generic `RequestScopedHost` but is cast to `ApplicationServicesFactory` to access `forSystemTask`; consider tightening the Cellix/timer registration types so the handler creator receives an `ApplicationServicesFactory` (or a host that explicitly exposes `forSystemTask`) and avoid the runtime cast.
- The `deleteByListing` application service runs a separate `withScopedTransaction` per conversation, which can become expensive for listings with many conversations; consider wrapping all deletions for a listing in a single unit-of-work/transaction where your domain and infrastructure allow it.
- The `processExpiredDeletions` loop deletes listings, conversations, and blobs fully sequentially; if you expect large batches, you might want to introduce bounded concurrency (e.g., per-listing or per-blob) to reduce total run time while still keeping per-listing error isolation.
## Individual Comments
### Comment 1
<location> `apps/api/src/timers/expired-listing-deletion-handler.ts:8-12` </location>
<code_context>
+
+const tracer = trace.getTracer('timer:expired-listing-deletion');
+
+export const expiredListingDeletionHandlerCreator = (
+ applicationServicesHost: RequestScopedHost<ApplicationServices, unknown>,
+): TimerHandler => {
+ return async (timer, context) => {
+ await tracer.startActiveSpan('processExpiredListingDeletions', async (span) => {
+ try {
+ context.log('ExpiredListingDeletion: Timer triggered');
</code_context>
<issue_to_address>
**issue (bug_risk):** Timer handler type forces an unsafe cast to `ApplicationServicesFactory`
The handler argument is declared as `RequestScopedHost<ApplicationServices, unknown>`, but then cast to `ApplicationServicesFactory` to call `forSystemTask()`. This hides the real requirement behind an unsafe cast. Please change the parameter type to something that natively exposes `forSystemTask` (e.g. `ApplicationServicesFactory` or a small interface with `forRequest` + `forSystemTask`) so the compiler enforces correct usage and the cast is unnecessary.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/persistence/src/datasources/readonly/conversation/conversation/conversation.read-repository.test.ts:50` </location>
<code_context>
}
function createNullPopulateChain<T>(result: T) {
- const innerLean = { lean: vi.fn(async () => result) };
- const innerPopulate = { populate: vi.fn(() => innerLean) };
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a single reusable query-mocking helper to replace the multiple bespoke mock builders added in these tests.
You can reduce the added complexity by unifying the query mocking into a single, minimal helper and reusing it across scenarios.
### 1. Replace `createNullPopulateChain` with a generic helper
Keep the thenable + `exec` behavior, but centralize it so you don’t need to redefine `createMockQuery` inline:
```ts
function createQueryChain<T>(result: T) {
const mockQuery = {
lean: vi.fn(),
populate: vi.fn(),
exec: vi.fn().mockResolvedValue(result),
catch: vi.fn((onReject) => Promise.resolve(result).catch(onReject)),
};
mockQuery.lean.mockReturnValue(mockQuery);
mockQuery.populate.mockReturnValue(mockQuery);
// biome-ignore lint/suspicious/noThenProperty: Intentional thenable mock for Mongoose queries
Object.defineProperty(mockQuery, 'then', {
value: vi.fn((onResolve, onReject) =>
Promise.resolve(result).then(onResolve, onReject)
),
enumerable: false,
configurable: true,
});
return mockQuery;
}
```
Then use it everywhere:
```ts
// old
mockModel.findOne = vi.fn(() => createNullPopulateChain(mockConversation)) as unknown as typeof mockModel.findOne;
// new
mockModel.findOne = vi.fn(() => createQueryChain(mockConversation)) as unknown as typeof mockModel.findOne;
// old
mockModel.find = vi.fn(() => createNullPopulateChain([])) as unknown as typeof mockModel.find;
// new
mockModel.find = vi.fn(() => createQueryChain([])) as unknown as typeof mockModel.find;
```
And in the “Getting conversations by listing ID” scenario, drop the inline `createMockQuery` and reuse the helper:
```ts
Given('a Conversation document with listing "listing-1"', () => {
mockConversations = [
makeMockConversation({
listing: makeMockListing('listing-1'),
}),
];
mockModel.find = vi.fn(
() => createQueryChain(mockConversations)
) as unknown as typeof mockModel.find;
});
```
This keeps all existing behavior (thenable, `exec`, `populate`, `lean`, `catch`) but avoids duplicated mocking logic and makes each test easier to read.
</issue_to_address>
### Comment 3
<location> `packages/sthrift/application-services/src/contexts/listing/item/index.ts:68` </location>
<code_context>
+ listingDeletionConfig?: ListingDeletionConfig;
}
export const ItemListing = (
- dataSources: DataSources,
+ deps: DataSources | ItemListingDependencies,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the union-type handling and default configuration into a small `normalizeItemListingDeps` helper so the `ItemListing` factory stays focused on simple wiring.
You can keep the backward-compatible union while isolating the shape‑normalization and defaulting logic in a tiny helper. That removes the runtime `'in'` checks from the main factory and makes the wiring easier to read.
For example:
```ts
type ItemListingDepsInput = DataSources | ItemListingDependencies;
const normalizeItemListingDeps = (
deps: ItemListingDepsInput,
): ItemListingDependencies => {
if ('dataSources' in deps) {
return {
dataSources: deps.dataSources,
blobStorage: deps.blobStorage,
listingDeletionConfig: deps.listingDeletionConfig ?? {
archivalMonths: 6,
batchSize: 100,
blobContainerName: 'listing-images',
},
};
}
// Backwards-compatible DataSources-only call
return {
dataSources: deps,
listingDeletionConfig: {
archivalMonths: 6,
batchSize: 100,
blobContainerName: 'listing-images',
},
};
};
```
Then the factory becomes focused on wiring:
```ts
export const ItemListing = (
deps: DataSources | ItemListingDependencies,
): ItemListingApplicationService => {
const { dataSources, blobStorage, listingDeletionConfig } =
normalizeItemListingDeps(deps);
return {
create: create(dataSources),
queryById: queryById(dataSources),
queryBySharer: queryBySharer(dataSources),
queryAll: queryAll(dataSources),
cancel: cancel(dataSources),
update: update(dataSources),
deleteListings: deleteListings(dataSources),
unblock: unblock(dataSources),
queryPaged: queryPaged(dataSources),
processExpiredDeletions: processExpiredDeletions(
dataSources,
listingDeletionConfig,
blobStorage,
),
};
};
```
This keeps all existing behavior and call sites intact but localizes the union-type introspection and default config to a single, easily testable function.
</issue_to_address>
### Comment 4
<location> `packages/sthrift/application-services/src/contexts/listing/index.ts:20` </location>
<code_context>
+ listingDeletionConfig?: ListingDeletionConfig;
+}
+
export const Listing = (
- dataSources: DataSources,
+ deps: DataSources | ListingDependencies,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the dependency-union handling into a dedicated `normalizeListingDeps` helper so that `Listing` remains a thin, easily readable wrapper.
You can hide the union-handling and repackaging behind a small helper to keep `Listing` as a simple thin wrapper.
```ts
interface ListingDependencies {
dataSources: DataSources;
blobStorage?: Domain.Services['BlobStorage'];
listingDeletionConfig?: ListingDeletionConfig;
}
const normalizeListingDeps = (
deps: DataSources | ListingDependencies,
): ItemListingDependencies => {
const dataSources = 'dataSources' in deps ? deps.dataSources : deps;
const blobStorage = 'blobStorage' in deps ? deps.blobStorage : undefined;
const listingDeletionConfig =
'listingDeletionConfig' in deps ? deps.listingDeletionConfig : undefined;
const itemListingDeps: ItemListingDependencies = { dataSources };
if (blobStorage) itemListingDeps.blobStorage = blobStorage;
if (listingDeletionConfig) {
itemListingDeps.listingDeletionConfig = listingDeletionConfig;
}
return itemListingDeps;
};
export const Listing = (
deps: DataSources | ListingDependencies,
): ListingContextApplicationService => {
const itemListingDeps = normalizeListingDeps(deps);
return {
ItemListing: ItemListingApi(itemListingDeps),
};
};
```
This keeps all current behavior (including the union input type and optional fields) but makes `Listing` itself easy to scan: it just normalizes deps and forwards them. The complexity of handling different shapes of `deps` is isolated and testable in `normalizeListingDeps`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...ence/src/datasources/readonly/conversation/conversation/conversation.read-repository.test.ts
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
conversation.read-repository.test.tstheGiven/Whenblock aroundgetBySharerReserverListinghas a misplaced});andWhen(...)on the same line, which will make the scenario hard to read and potentially brittle; please fix the formatting so each step is clearly separated and scoped. - In
process-expired-deletions.ts, the comment mentions 'bounded concurrency' for image deletion but the implementation fires alldeleteBlobcalls in parallel withPromise.allSettled; either implement an actual concurrency limit or adjust the comment to reflect the current behavior to avoid confusion for future maintainers. - The timer registration path (
registerAzureFunctionTimerHandler→expired-listing-deletion-handler) currently relies on casting the handler creator toRequestScopedHostand usingApplicationServicesFactoryas the argument type; consider tightening these types (e.g., basing both onRequestScopedHost<AppServices, unknown>) to remove the cast and make the expected shape of the host explicit at compile time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `conversation.read-repository.test.ts` the `Given`/`When` block around `getBySharerReserverListing` has a misplaced `});` and `When(...)` on the same line, which will make the scenario hard to read and potentially brittle; please fix the formatting so each step is clearly separated and scoped.
- In `process-expired-deletions.ts`, the comment mentions 'bounded concurrency' for image deletion but the implementation fires all `deleteBlob` calls in parallel with `Promise.allSettled`; either implement an actual concurrency limit or adjust the comment to reflect the current behavior to avoid confusion for future maintainers.
- The timer registration path (`registerAzureFunctionTimerHandler` → `expired-listing-deletion-handler`) currently relies on casting the handler creator to `RequestScopedHost` and using `ApplicationServicesFactory` as the argument type; consider tightening these types (e.g., basing both on `RequestScopedHost<AppServices, unknown>`) to remove the cast and make the expected shape of the host explicit at compile time.
## Individual Comments
### Comment 1
<location> `apps/api/src/cellix.ts:192-195` </location>
<code_context>
handlerCreator: (applicationServicesHost: AppHost<AppServices>) => HttpHandler;
}
+interface PendingTimerHandler<AppServices> {
+ name: string;
+ schedule: string;
+ handlerCreator: (applicationServicesHost: RequestScopedHost<AppServices, unknown>) => TimerHandler;
+}
+
</code_context>
<issue_to_address>
**suggestion:** The timer handler typing and casting hide the `forSystemTask` requirement and weaken type-safety.
`PendingTimerHandler` stores `handlerCreator` as `(host: RequestScopedHost<AppServices, unknown>) => TimerHandler`, and `registerAzureFunctionTimerHandler` casts any handler factory to that type. But `expiredListingDeletionHandlerCreator` expects an `ApplicationServicesFactory` with `.forSystemTask()`, which `RequestScopedHost` doesn’t guarantee. The cast hides this mismatch and can allow invalid factories that only fail at runtime.
To keep this type-safe, either:
- Define a `TimerAppServicesHost` that extends `RequestScopedHost` with `forSystemTask` and use it in the timer handler typing, or
- Make `PendingTimerHandler` generic over `TFactory` (as with `registerAzureFunctionHttpHandler`) and carry that type instead of downcasting.
This ensures handlers that need `forSystemTask` are correctly constrained by the type system.
Suggested implementation:
```typescript
export type RequestScopedHost<S, H = unknown> = {
forRequest(rawAuthHeader?: string, hints?: H): Promise<S>;
};
export type TimerAppServicesHost<AppServices, H = unknown> =
RequestScopedHost<AppServices, H> & {
/**
* Returns application services configured for background/system tasks
* where there is no end-user request context.
*/
forSystemTask(): Promise<AppServices>;
};
```
```typescript
interface PendingTimerHandler<AppServices> {
name: string;
schedule: string;
handlerCreator: (applicationServicesHost: TimerAppServicesHost<AppServices>) => TimerHandler;
}
```
To fully apply the type-safe `forSystemTask` constraint, you should also:
1. Update the signature of `registerAzureFunctionTimerHandler` (or equivalent) so its `handlerCreator` parameter and any stored timer handler metadata use `TimerAppServicesHost<AppServices>` instead of `RequestScopedHost<AppServices, unknown>`, e.g.:
- From:
`(handlerCreator: (host: RequestScopedHost<AppServices, unknown>) => TimerHandler) => ...`
- To:
`(handlerCreator: (host: TimerAppServicesHost<AppServices>) => TimerHandler) => ...`
2. Remove any casts that currently coerce a more specific factory (e.g. one requiring `.forSystemTask()`) to `(host: RequestScopedHost<AppServices, unknown>) => TimerHandler`. They should become unnecessary once all timer-related types use `TimerAppServicesHost`.
3. Ensure that whatever object you pass to `handlerCreator` when a timer fires actually implements `TimerAppServicesHost<AppServices>`:
- It must already satisfy `RequestScopedHost<AppServices, H>`.
- It must also define `forSystemTask(): Promise<AppServices>`.
If there is an existing `ApplicationServicesFactory`/host that has `forSystemTask`, it is likely the right implementation type to wire in here.
4. If there are other helper types or registries that currently reference `PendingTimerHandler<AppServices>` or the old `(RequestScopedHost<...>) => TimerHandler` signature, update them to propagate `TimerAppServicesHost<AppServices>` so the type-safety is enforced end-to-end.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/application-services/src/contexts/listing/item/process-expired-deletions.ts:16-25` </location>
<code_context>
+ errors: Array<{ listingId: string; error: string }>;
+}
+
+async function deleteListingImages(
+ blobStorage: BlobStorageService,
+ images: string[],
+ containerName: string,
+): Promise<number> {
+ let deletedCount = 0;
+ // Process images concurrently with bounded concurrency
+ const imagePromises = images.map((imagePath) =>
+ blobStorage
+ .deleteBlob(containerName, imagePath)
+ .then(() => ({ success: true as const, imagePath }))
+ .catch((error) => ({
+ success: false as const,
+ imagePath,
+ error:
+ error instanceof Error ? error.message : String(error),
+ })),
+ );
+
+ const results = await Promise.allSettled(imagePromises);
+ for (const result of results) {
+ if (result.status === 'fulfilled') {
</code_context>
<issue_to_address>
**suggestion:** The image deletion helper both double-wraps errors and doesn’t actually implement bounded concurrency as the comment suggests.
Because the `catch` is handled inside the `images.map`, each promise resolves to `{ success, imagePath, error? }` and never rejects, so `Promise.allSettled` will only ever yield `'fulfilled'` entries; the `'rejected'` branch can’t be hit.
The “bounded concurrency” note in the comment is also inaccurate: `Promise.allSettled(imagePromises)` fires all deletions simultaneously.
To align behavior with the current structure, you can await the promises directly and branch on `success`:
```ts
const results = await Promise.all(imagePromises);
for (const r of results) {
if (r.success) {
deletedCount++;
} else {
console.warn(
`[ExpiredDeletion] Failed to delete image ${r.imagePath}: ${r.error}`,
);
}
}
```
If true bounded concurrency is desired (e.g. to avoid overloading storage on large batches), consider using a concurrency limiter like `p-limit` and update the comment to match the actual behavior.
</issue_to_address>
### Comment 3
<location> `apps/api/src/cellix.ts:223` </location>
<code_context>
private readonly tracer: Tracer;
private readonly servicesInternal: Map<ServiceKey<ServiceBase>, ServiceBase> = new Map();
private readonly pendingHandlers: Array<PendingHandler<AppServices>> = [];
+ private readonly pendingTimerHandlers: Array<PendingTimerHandler<AppServices>> = [];
private serviceInitializedInternal = false;
private phase: Phase = 'infrastructure';
</code_context>
<issue_to_address>
**issue (complexity):** Consider unifying HTTP and timer function registration into a single abstraction and simplifying the timer factory type to keep the lifecycle coherent and avoid unnecessary generic complexity.
You’ve effectively introduced a second, parallel abstraction for timers. You can keep all behavior and reduce complexity by unifying the pending registrations and simplifying the factory type.
### 1. Unify HTTP and timer registrations
Instead of `PendingHandler` + `PendingTimerHandler` + two arrays + two loops, use a single discriminated union:
```ts
type PendingFunction<AppServices> =
| {
kind: 'http';
name: string;
options: Omit<HttpFunctionOptions, 'handler'>;
handlerCreator: (host: AppHost<AppServices>) => HttpHandler;
}
| {
kind: 'timer';
name: string;
schedule: string;
handlerCreator: (host: AppHost<AppServices>) => TimerHandler;
};
```
Then replace:
```ts
private readonly pendingHandlers: Array<PendingHandler<AppServices>> = [];
private readonly pendingTimerHandlers: Array<PendingTimerHandler<AppServices>> = [];
```
with:
```ts
private readonly pendingFunctions: Array<PendingFunction<AppServices>> = [];
```
And change the registration methods to push into a single list:
```ts
public registerAzureFunctionHttpHandler(
name: string,
options: Omit<HttpFunctionOptions, 'handler'>,
handlerCreator: (host: AppHost<AppServices>) => HttpHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingFunctions.push({ kind: 'http', name, options, handlerCreator });
this.phase = 'handlers';
return this;
}
public registerAzureFunctionTimerHandler(
name: string,
schedule: string,
handlerCreator: (host: AppHost<AppServices>) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingFunctions.push({ kind: 'timer', name, schedule, handlerCreator });
this.phase = 'handlers';
return this;
}
```
And finally, collapse the two loops in `setupLifecycle` into one:
```ts
private setupLifecycle(): void {
for (const f of this.pendingFunctions) {
if (!this.appServicesHostInternal) {
throw new Error('Application not started yet');
}
switch (f.kind) {
case 'http':
app.http(f.name, {
...f.options,
handler: (request, context) =>
f.handlerCreator(this.appServicesHostInternal!)(request, context),
});
break;
case 'timer':
app.timer(f.name, {
schedule: f.schedule,
handler: (timer, context) =>
f.handlerCreator(this.appServicesHostInternal!)(timer, context),
});
break;
}
}
// existing app.hook.appStart / app.hook.appTerminate ...
}
```
This keeps all existing behavior but removes duplicated structures and keeps the lifecycle for all functions in one coherent place.
### 2. Simplify the timer factory type
The generic `TFactory extends RequestScopedHost<AppServices, unknown> = AppHost<AppServices>` plus the cast back to `RequestScopedHost` suggests the generic isn’t providing real flexibility.
Unless you have a concrete use case that genuinely needs a narrower `RequestScopedHost` subtype here, you can simplify and avoid the runtime cast:
```ts
public registerAzureFunctionTimerHandler(
name: string,
schedule: string,
handlerCreator: (host: AppHost<AppServices>) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingFunctions.push({ kind: 'timer', name, schedule, handlerCreator });
this.phase = 'handlers';
return this;
}
```
This aligns the timer handler factory with the HTTP handler factory (both take `AppHost<AppServices>`), and keeps `RequestScopedHost` as an implementation detail of the hosting model rather than part of the timer API surface.
</issue_to_address>
### Comment 4
<location> `packages/sthrift/application-services/src/contexts/listing/item/index.ts:68` </location>
<code_context>
+ listingDeletionConfig?: ListingDeletionConfig;
}
export const ItemListing = (
- dataSources: DataSources,
+ deps: DataSources | ItemListingDependencies,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the ItemListing factory by using a single dependency object and delegating config defaulting to processExpiredDeletions (or a helper) instead of handling both concerns inline.
You can reduce the new complexity by separating “backwards-compatibility wiring” from the core factory and by centralizing the config defaulting logic.
### 1. Keep `ItemListing` focused on a single dependency shape
Instead of overloading the factory with `DataSources | ItemListingDependencies` and doing runtime discrimination, keep the factory typed to one dependency object and put the adapter near the composition root:
```ts
// item-listing.ts
export interface ItemListingDependencies {
dataSources: DataSources;
blobStorage?: Domain.Services['BlobStorage'];
listingDeletionConfig?: ListingDeletionConfig;
}
export const ItemListing = (
deps: ItemListingDependencies,
): ItemListingApplicationService => {
const { dataSources, blobStorage, listingDeletionConfig } = deps;
return {
create: create(dataSources),
queryById: queryById(dataSources),
queryBySharer: queryBySharer(dataSources),
queryAll: queryAll(dataSources),
cancel: cancel(dataSources),
update: update(dataSources),
deleteListings: deleteListings(dataSources),
unblock: unblock(dataSources),
queryPaged: queryPaged(dataSources),
processExpiredDeletions: processExpiredDeletions(
dataSources,
listingDeletionConfig,
blobStorage,
),
};
}
```
Then add a tiny adapter for legacy callers that only have `DataSources`:
```ts
// composition-root / adapter
export const ItemListingFromDataSources = (dataSources: DataSources) =>
ItemListing({ dataSources });
```
Existing code can switch to `ItemListingFromDataSources(dataSources)` with no behavior change, and the core factory stays simple and type-safe.
### 2. Move deletion defaults out of the factory
Instead of computing defaults inside `ItemListing`, let `processExpiredDeletions` (or a helper it calls) handle the defaulting:
```ts
// process-expired-deletions.ts
const withDefaultDeletionConfig = (
config?: ListingDeletionConfig,
): ListingDeletionConfig => ({
archivalMonths: 6,
batchSize: 100,
blobContainerName: 'listing-images',
...config,
});
export const processExpiredDeletions = (
dataSources: DataSources,
config: ListingDeletionConfig | undefined,
blobStorage?: Domain.Services['BlobStorage'],
): ProcessExpiredDeletionsResult => {
const effectiveConfig = withDefaultDeletionConfig(config);
// ... existing logic using effectiveConfig and blobStorage
};
```
This keeps `ItemListing` responsible only for wiring dependencies, not for mixing dependency resolution and configuration policy, while preserving the current behavior (defaults + optional overrides).
</issue_to_address>
### Comment 5
<location> `packages/sthrift/application-services/src/contexts/listing/index.ts:20` </location>
<code_context>
+ listingDeletionConfig?: ListingDeletionConfig;
+}
+
export const Listing = (
- dataSources: DataSources,
+ deps: DataSources | ListingDependencies,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the dependency shape branching into a small `normalizeListingDeps` helper so the `Listing` factory only does straightforward wiring.
The main complexity comes from `Listing` doing both overload discrimination and wiring. You can keep the current capabilities but move the branching into a small normalizer so the context factory stays simple.
For example:
```ts
type ListingInput = DataSources | ListingDependencies;
const normalizeListingDeps = (deps: ListingInput): ItemListingDependencies => {
if ('dataSources' in deps) {
const { dataSources, blobStorage, listingDeletionConfig } = deps;
return {
dataSources,
blobStorage,
listingDeletionConfig,
};
}
// Backwards-compatible path: plain DataSources
return { dataSources: deps };
};
```
Then `Listing` becomes straightforward wiring:
```ts
export const Listing = (
deps: DataSources | ListingDependencies,
): ListingContextApplicationService => {
const itemListingDeps = normalizeListingDeps(deps);
return {
ItemListing: ItemListingApi(itemListingDeps),
};
};
```
This keeps the public signature and behavior exactly as-is, but removes the repeated `'in'` checks and object mutation from the main factory, making it easier to read and reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…-deletions and conversation read repository tests - Updated comments for clarity in process-expired-deletions.ts - Adjusted formatting for better readability in conversation.read-repository.test.ts - Consolidated import statements in conversation.read-repository.test.ts - Enhanced error handling and test scenarios for missing IDs and invalid ObjectId formats
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
processExpiredDeletions,deleteListingImagesperforms unbounded parallel blob deletions for each listing; consider adding a simple concurrency limiter (e.g., chunking or a small pool) to avoid overloading storage or hitting throttling limits when many images are present. - Timer-driven and repository flows now use a mix of
console.log/console.errorwith structured payloads for errors; you might want to route these through a common logging abstraction (or at least standardize message formats) so operational logs from deletions and read repositories are easier to correlate and filter.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `processExpiredDeletions`, `deleteListingImages` performs unbounded parallel blob deletions for each listing; consider adding a simple concurrency limiter (e.g., chunking or a small pool) to avoid overloading storage or hitting throttling limits when many images are present.
- Timer-driven and repository flows now use a mix of `console.log`/`console.error` with structured payloads for errors; you might want to route these through a common logging abstraction (or at least standardize message formats) so operational logs from deletions and read repositories are easier to correlate and filter.
## Individual Comments
### Comment 1
<location> `packages/sthrift/persistence/src/datasources/readonly/reservation-request/reservation-request/reservation-request.read-repository.test.ts:51-60` </location>
<code_context>
- const innerPopulate = { populate: vi.fn(() => innerLean) };
- const outerPopulate = { populate: vi.fn(() => innerPopulate) };
- return { populate: vi.fn(() => outerPopulate) };
+function createQueryChain<T>(result: T) {
+ const mockQuery = {
+ lean: vi.fn(),
+ populate: vi.fn(),
+ exec: vi.fn().mockResolvedValue(result),
+ catch: vi.fn((onReject) => Promise.resolve(result).catch(onReject)),
+ };
+
+ mockQuery.lean.mockReturnValue(mockQuery);
+ mockQuery.populate.mockReturnValue(mockQuery);
+
+ // biome-ignore lint/suspicious/noThenProperty: Intentional thenable mock for Mongoose queries
+ Object.defineProperty(mockQuery, 'then', {
+ value: vi.fn((onResolve, onReject) =>
+ Promise.resolve(result).then(onResolve, onReject),
+ ),
+ enumerable: false,
+ configurable: true,
+ });
+
+ return mockQuery;
}
</code_context>
<issue_to_address>
**issue (testing):** The new query mock is missing `sort` and `limit`, which may break tests if the repository chains those methods.
The previous `createMockQuery` also mocked `sort` and `limit`, implying the repository calls them on the `find` query. The new `createQueryChain` only mocks `lean`, `populate`, `exec`, and `catch`, so any remaining `find(...).sort(...).limit(...).lean()` usage will now fail with `sort/limit is not a function`.
Please add `sort` and `limit` to this mock (returning `mockQuery` for chaining), unless the repository implementation has definitively stopped using them.
</issue_to_address>
### Comment 2
<location> `apps/api/src/cellix.ts:136` </location>
<code_context>
+ *
+ * @throws Error - If called before application services are initialized.
+ */
+ registerAzureFunctionTimerHandler<
+ TFactory extends RequestScopedHost<
+ AppServices,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the timer handler typing, extracting shared handler registration helpers, and flattening the tracing loop to reduce type noise and improve readability without changing behaviour.
You can simplify a few spots without changing behaviour:
### 1. Over‑generic `registerAzureFunctionTimerHandler`
`TFactory` is constrained to `RequestScopedHost<AppServices, unknown>` but then cast back to `PendingTimerHandler<AppServices>['handlerCreator']`, and at runtime you always use `this.appServicesHostInternal`. You can drop the generic and align the timer API with HTTP:
```ts
// Keep AppHost alias as-is
type AppHost<AppServices> = RequestScopedHost<AppServices, unknown>;
interface AzureFunctionHandlerRegistry<ContextType = unknown, AppServices = unknown> {
// ...
registerAzureFunctionTimerHandler(
name: string,
schedule: string,
handlerCreator: (applicationServicesHost: AppHost<AppServices>) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices>;
}
```
```ts
interface PendingTimerHandler<AppServices> {
name: string;
schedule: string;
handlerCreator: (applicationServicesHost: AppHost<AppServices>) => TimerHandler;
}
public registerAzureFunctionTimerHandler(
name: string,
schedule: string,
handlerCreator: (applicationServicesHost: AppHost<AppServices>) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingTimerHandlers.push({ name, schedule, handlerCreator });
this.phase = 'handlers';
return this;
}
```
This removes the type erasure comment and cast while keeping the same runtime semantics.
### 2. Parallel HTTP / timer registration
The HTTP and timer registration loops are now almost identical. A small internal helper keeps `setupLifecycle` flatter and makes adding future trigger types easier:
```ts
private registerHttpHandlers(): void {
for (const h of this.pendingHandlers) {
app.http(h.name, {
...h.options,
handler: (request, context) => {
if (!this.appServicesHostInternal) {
throw new Error('Application not started yet');
}
return h.handlerCreator(this.appServicesHostInternal)(request, context);
},
});
}
}
private registerTimerHandlers(): void {
for (const t of this.pendingTimerHandlers) {
app.timer(t.name, {
schedule: t.schedule,
handler: (timer, context) => {
if (!this.appServicesHostInternal) {
throw new Error('Application not started yet');
}
return t.handlerCreator(this.appServicesHostInternal)(timer, context);
},
});
}
}
private setupLifecycle(): void {
this.registerHttpHandlers();
this.registerTimerHandlers();
// appStart / appTerminate unchanged...
}
```
You still have explicit hooks per trigger, but the main lifecycle method stays simpler.
### 3. Tracing loop readability
`iterateServicesWithTracing` is now quite dense with inline template strings and name extraction. Precomputing values per service flattens the span callback:
```ts
private async iterateServicesWithTracing(
operationName: 'start' | 'stop',
serviceMethod: 'startUp' | 'shutDown',
): Promise<void> {
const operationFullName = `${operationName.charAt(0).toUpperCase() + operationName.slice(1)}Service`;
const operationActionPending = operationName === 'start' ? 'starting' : 'stopping';
const operationActionCompleted = operationName === 'start' ? 'started' : 'stopped';
await Promise.all(
Array.from(this.servicesInternal.entries()).map(([ctor, service]) => {
const ctorName = (ctor as { name?: string }).name ?? 'Service';
const spanName = `Service ${ctorName} ${operationName}`;
const startMsg = `${operationFullName}: Service ${ctorName} ${operationActionPending}`;
const doneMsg = `${operationFullName}: Service ${ctorName} ${operationActionCompleted}`;
return this.tracer.startActiveSpan(spanName, async (span) => {
try {
console.log(startMsg);
await service[serviceMethod]();
span.setStatus({ code: SpanStatusCode.OK, message: doneMsg });
console.log(doneMsg);
} catch (err) {
span.setStatus({ code: SpanStatusCode.ERROR });
if (err instanceof Error) {
span.recordException(err);
}
throw err;
} finally {
span.end();
}
});
}),
);
}
```
Same tracing semantics, but the control flow inside the span callback is easier to scan.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ng to avoid throttling
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The
createQueryChainhelper for mocking Mongoose queries is now duplicated across multiple test files; consider extracting a shared test utility (e.g., in a test helpers module) to avoid repetition and keep the mocking behavior consistent. - The timer handler currently depends on
ApplicationServicesFactorywhile HTTP/GraphQL handlers consume the more genericAppServicesHost<ApplicationServices>; it may be worth aligning these so all handlers share the same host abstraction (e.g., exposingforSystemTask()off the generic host type) for consistency and easier future refactoring.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `createQueryChain` helper for mocking Mongoose queries is now duplicated across multiple test files; consider extracting a shared test utility (e.g., in a test helpers module) to avoid repetition and keep the mocking behavior consistent.
- The timer handler currently depends on `ApplicationServicesFactory` while HTTP/GraphQL handlers consume the more generic `AppServicesHost<ApplicationServices>`; it may be worth aligning these so all handlers share the same host abstraction (e.g., exposing `forSystemTask()` off the generic host type) for consistency and easier future refactoring.
## Individual Comments
### Comment 1
<location> `packages/sthrift/persistence/src/datasources/readonly/listing/item/item-listing.read-repository.ts:200-209` </location>
<code_context>
}
}
+
+ async getExpiredForDeletion(
+ archivalMonths: number,
+ limit = 100,
+ ): Promise<Domain.Contexts.Listing.ItemListing.ItemListingEntityReference[]> {
+ // Validate archivalMonths to avoid unexpected date calculation when misconfigured
+ if (!Number.isFinite(archivalMonths) || archivalMonths <= 0) {
+ console.error(
+ '[ItemListingReadRepository] Invalid archivalMonths value:',
+ {
+ archivalMonths,
+ isFinite: Number.isFinite(archivalMonths),
+ },
+ );
+ return [];
+ }
+
+ const cutoffDate = new Date();
+ cutoffDate.setMonth(cutoffDate.getMonth() - archivalMonths);
+
+ const result = await this.mongoDataSource.find(
+ {
+ state: { $in: ['Expired', 'Cancelled'] },
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Validate and sanitize the `limit` parameter before passing it to the data source
`archivalMonths` is validated but `limit` is passed directly to `mongoDataSource.find`. Values like `NaN`, negative numbers, or `Infinity` (e.g., from a bad env var) are accepted by Mongoose with driver-dependent behavior and could result in unbounded queries. Normalize `limit` first (e.g., default on invalid and clamp to a max, such as 100–1000) before passing it to the data source.
</issue_to_address>
### Comment 2
<location> `packages/sthrift/application-services/src/contexts/listing/item/process-expired-deletions.ts:37-48` </location>
<code_context>
+ const CHUNK_SIZE = 5; // Process 5 images at a time to avoid throttling
+ let deletedCount = 0;
+
+ for (let i = 0; i < images.length; i += CHUNK_SIZE) {
+ const chunk = images.slice(i, i + CHUNK_SIZE);
+ const chunkPromises = chunk.map((imagePath) =>
+ blobStorage
+ .deleteBlob(containerName, imagePath)
+ .then(() => ({ success: true as const, imagePath }))
+ .catch((error) => ({
+ success: false as const,
+ imagePath,
+ error: error instanceof Error ? error.message : String(error),
+ })),
+ );
+
+ const results = await Promise.allSettled(chunkPromises);
+ for (const result of results) {
+ if (result.status === 'fulfilled') {
</code_context>
<issue_to_address>
**suggestion:** Avoid wrapping already-caught promises in `Promise.allSettled`, or propagate rejections more explicitly
Because each `deleteBlob` call maps errors into `{ success: false, ... }`, `chunkPromises` never reject and `Promise.allSettled` adds no value—the `'rejected'` branch is dead code.
You can either:
1) Switch to `await Promise.all(chunkPromises)` and remove the `status === 'rejected'` handling; or
2) Remove the inner `.catch`, let deletions reject, and keep `allSettled` to handle actual rejections.
Using a single, consistent pattern will make the error-handling behavior clearer and less surprising.
```suggestion
const results = await Promise.all(chunkPromises);
for (const result of results) {
if (result.success) {
deletedCount++;
} else {
console.warn(
`[ExpiredDeletion] Failed to delete image ${result.imagePath}: ${result.error}`,
);
}
}
```
</issue_to_address>
### Comment 3
<location> `apps/api/src/cellix.ts:370` </location>
<code_context>
return this;
}
+ public registerAzureFunctionTimerHandler<
+ TFactory extends RequestScopedHost<
+ AppServices,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new timer registration API by using a single concrete host type and a shared helper for host-guarded handler registration instead of generic + duplicated logic.
You can keep all the new functionality but significantly reduce complexity by tightening the types and deduplicating the registration paths.
### 1. Remove the generic + cast from `registerAzureFunctionTimerHandler`
The `TFactory` generic is immediately erased via a cast to `PendingTimerHandler<AppServices>['handlerCreator']`, so it adds complexity without real extra power. You already use `AppHost<AppServices>` for HTTP; you can mirror that for timers.
**Before:**
```ts
registerAzureFunctionTimerHandler<
TFactory extends RequestScopedHost<AppServices, unknown> = AppHost<AppServices>,
>(
name: string,
schedule: string,
handlerCreator: (applicationServicesHost: TFactory) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingTimerHandlers.push({
name,
schedule,
handlerCreator:
handlerCreator as PendingTimerHandler<AppServices>['handlerCreator'],
});
this.phase = 'handlers';
return this;
}
```
**After (same functionality, simpler types):**
```ts
public registerAzureFunctionTimerHandler(
name: string,
schedule: string,
handlerCreator: (
applicationServicesHost: AppHost<AppServices>,
) => TimerHandler,
): AzureFunctionHandlerRegistry<ContextType, AppServices> {
this.ensurePhase('app-services', 'handlers');
this.pendingTimerHandlers.push({ name, schedule, handlerCreator });
this.phase = 'handlers';
return this;
}
```
And adjust `PendingTimerHandler` to match:
```ts
interface PendingTimerHandler<AppServices> {
name: string;
schedule: string;
handlerCreator: (applicationServicesHost: AppHost<AppServices>) => TimerHandler;
}
```
This removes the type-erasure cast and keeps the mental model unified: “a handler creator always takes an `AppHost<AppServices>`”.
---
### 2. Share registration logic for HTTP and Timer handlers
The `setupLifecycle` loops are almost identical. A small private helper keeps the behavior the same but reduces duplication and the “parallel system” feel.
**Before (two separate loops):**
```ts
for (const h of this.pendingHandlers) {
app.http(h.name, {
...h.options,
handler: (request, context) => {
if (!this.appServicesHostInternal) {
throw new Error('Application not started yet');
}
return h.handlerCreator(this.appServicesHostInternal)(request, context);
},
});
}
for (const t of this.pendingTimerHandlers) {
app.timer(t.name, {
schedule: t.schedule,
handler: (timer, context) => {
if (!this.appServicesHostInternal) {
throw new Error('Application not started yet');
}
return t.handlerCreator(this.appServicesHostInternal)(timer, context);
},
});
}
```
**After (shared helper, same behavior):**
```ts
private withStartedHost<Args extends unknown[], R>(
fn: (host: AppHost<AppServices>, ...args: Args) => R,
): (...args: Args) => R {
return (...args: Args) => {
if (!this.appServicesHostInternal) {
throw new Error('Application not started yet');
}
return fn(this.appServicesHostInternal, ...args);
};
}
private setupLifecycle(): void {
for (const h of this.pendingHandlers) {
app.http(h.name, {
...h.options,
handler: this.withStartedHost((host, request, context) =>
h.handlerCreator(host)(request, context),
),
});
}
for (const t of this.pendingTimerHandlers) {
app.timer(t.name, {
schedule: t.schedule,
handler: this.withStartedHost((host, timer, context) =>
t.handlerCreator(host)(timer, context),
),
});
}
// ... appStart / appTerminate remain unchanged
}
```
This keeps the runtime behavior identical, centralizes the “ensure host is ready” logic, and makes adding future function types (queues, events, etc.) a small extension instead of duplicating boilerplate.
</issue_to_address>
### Comment 4
<location> `packages/sthrift/application-services/src/contexts/listing/item/process-expired-deletions.ts:64` </location>
<code_context>
+ });
+}
+
+export const processExpiredDeletions = (
+ dataSources: DataSources,
+ config: ListingDeletionConfig,
</code_context>
<issue_to_address>
**issue (complexity):** Consider flattening the main function and extracting per-listing and image-deletion helpers to make the control flow and error handling easier to scan and maintain.
You can reduce complexity in a few focused places without changing behavior.
### 1. Flatten `processExpiredDeletions` into a plain async function
The factory style adds an extra level for readers/tests without much benefit. You can keep the same behavior in the timer handler by closing over arguments there instead of here.
```ts
export async function processExpiredDeletions(
dataSources: DataSources,
config: ListingDeletionConfig,
blobStorage?: BlobStorageService,
): Promise<ProcessExpiredDeletionsResult> {
const result: ProcessExpiredDeletionsResult = {
deletedCount: 0,
deletedListingIds: [],
deletedConversationsCount: 0,
deletedImagesCount: 0,
errors: [],
};
const expiredListings =
await dataSources.readonlyDataSource.Listing.ItemListing.ItemListingReadRepo
.getExpiredForDeletion(config.archivalMonths, config.batchSize);
if (expiredListings.length === 0) {
return result;
}
const uow =
dataSources.domainDataSource.Listing.ItemListing.ItemListingUnitOfWork;
const deleteConversations = deleteConversationsByListing(dataSources);
for (const listing of expiredListings) {
const listingResult = await deleteSingleListing(
listing,
{ blobStorage, config, uow, deleteConversations },
);
mergeListingResult(result, listingResult);
}
return result;
}
```
And in the timer handler you can keep the existing “factory” behavior if desired:
```ts
// before: const handler = processExpiredDeletions(dataSources, config, blobStorage);
const handler = () => processExpiredDeletions(dataSources, config, blobStorage);
```
### 2. Extract per-listing orchestration
Pulling the per-listing flow into its own helper flattens the main function and localizes try/catch and logging:
```ts
interface SingleListingResult extends ProcessExpiredDeletionsResult {}
async function deleteSingleListing(
listing: { id: string; images?: string[] },
deps: {
blobStorage?: BlobStorageService;
config: ListingDeletionConfig;
uow: DataSources['domainDataSource']['Listing']['ItemListing']['ItemListingUnitOfWork'];
deleteConversations: (listingId: string) => Promise<{ deletedCount: number; errors: unknown[] }>;
},
): Promise<SingleListingResult> {
const { blobStorage, config, uow, deleteConversations } = deps;
const listingId = listing.id;
const result: SingleListingResult = {
deletedCount: 0,
deletedListingIds: [],
deletedConversationsCount: 0,
deletedImagesCount: 0,
errors: [],
};
try {
if (blobStorage && listing.images && listing.images.length > 0) {
const imagesDeleted = await deleteListingImages(
blobStorage,
listing.images,
config.blobContainerName,
);
result.deletedImagesCount += imagesDeleted;
console.log(`[ExpiredDeletion] Deleted ${imagesDeleted} images for listing ${listingId}`);
}
const conversationResult = await deleteConversations(listingId);
result.deletedConversationsCount += conversationResult.deletedCount;
if (conversationResult.errors.length > 0) {
console.warn(
`[ExpiredDeletion] Errors deleting conversations for listing ${listingId}: ${JSON.stringify(conversationResult.errors)}`,
);
}
await deleteListingById(uow, listingId);
result.deletedCount++;
result.deletedListingIds.push(listingId);
console.log(`[ExpiredDeletion] Deleted listing: ${listingId}`);
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error);
result.errors.push({ listingId, error: errorMessage });
console.error(
`[ExpiredDeletion] Failed to delete listing ${listingId}: ${errorMessage}`,
);
}
return result;
}
function mergeListingResult(
aggregate: ProcessExpiredDeletionsResult,
listingResult: ProcessExpiredDeletionsResult,
): void {
aggregate.deletedCount += listingResult.deletedCount;
aggregate.deletedListingIds.push(...listingResult.deletedListingIds);
aggregate.deletedConversationsCount += listingResult.deletedConversationsCount;
aggregate.deletedImagesCount += listingResult.deletedImagesCount;
aggregate.errors.push(...listingResult.errors);
}
```
This keeps all the same steps but makes the main flow “fetch -> loop -> merge” much easier to scan.
### 3. Simplify `deleteListingImages`
You can keep chunking but avoid the double-layer wrapping plus `allSettled`, and just log errors in place:
```ts
async function deleteListingImages(
blobStorage: BlobStorageService,
images: string[],
containerName: string,
): Promise<number> {
const CHUNK_SIZE = 5;
let deletedCount = 0;
for (let i = 0; i < images.length; i += CHUNK_SIZE) {
const chunk = images.slice(i, i + CHUNK_SIZE);
const results = await Promise.all(
chunk.map(async (imagePath) => {
try {
await blobStorage.deleteBlob(containerName, imagePath);
return true;
} catch (error) {
const message = error instanceof Error ? error.message : String(error);
console.warn(
`[ExpiredDeletion] Failed to delete image ${imagePath}: ${message}`,
);
return false;
}
}),
);
deletedCount += results.filter(Boolean).length;
}
return deletedCount;
}
```
Same behavior (chunked, per-image logging, success counting) with fewer branches and a simpler result shape.
### 4. Optional: move aggregate logging to caller
Since `ProcessExpiredDeletionsResult` already carries full stats and errors, the final summary log can be moved out to the caller (e.g., the timer handler). That leaves this module focused on behavior, while the trigger decides *how* to log:
```ts
// timer handler
const result = await processExpiredDeletions(dataSources, config, blobStorage);
console.log(
`[ExpiredDeletion] Completed: ${result.deletedCount} listings deleted, ` +
`${result.deletedConversationsCount} conversations deleted, ` +
`${result.deletedImagesCount} images deleted, ${result.errors.length} errors`,
);
```
This reduces side effects in the core use case without changing its external behavior.
</issue_to_address>
### Comment 5
<location> `packages/sthrift/application-services/src/contexts/listing/item/index.ts:68` </location>
<code_context>
+ listingDeletionConfig?: ListingDeletionConfig;
}
export const ItemListing = (
- dataSources: DataSources,
+ deps: DataSources | ItemListingDependencies,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a dependency normalization helper and a default config constant so the `ItemListing` factory remains simple and focused on wiring behavior.
You can reduce the added complexity by isolating the compatibility shim and default config instead of inlining both in `ItemListing`.
1. Extract a single normalization function for dependencies
Keep the public signature backward-compatible (`DataSources | ItemListingDependencies`), but move the `'dataSources' in deps` checks into a small helper so the factory only works with a single shape:
```ts
export type ItemListingDepsInput = DataSources | ItemListingDependencies;
const normalizeItemListingDeps = (
deps: ItemListingDepsInput,
): ItemListingDependencies => {
if ('dataSources' in deps) {
return {
dataSources: deps.dataSources,
blobStorage: deps.blobStorage,
listingDeletionConfig: deps.listingDeletionConfig,
};
}
// legacy callers passing `DataSources`
return { dataSources: deps };
};
```
Then `ItemListing` becomes simpler:
```ts
export const ItemListing = (
deps: ItemListingDepsInput,
): ItemListingApplicationService => {
const { dataSources, blobStorage, listingDeletionConfig } =
normalizeItemListingDeps(deps);
const deletionConfig = listingDeletionConfig ?? DEFAULT_LISTING_DELETION_CONFIG;
return {
create: create(dataSources),
queryById: queryById(dataSources),
queryBySharer: queryBySharer(dataSources),
queryAll: queryAll(dataSources),
cancel: cancel(dataSources),
update: update(dataSources),
deleteListings: deleteListings(dataSources),
unblock: unblock(dataSources),
queryPaged: queryPaged(dataSources),
processExpiredDeletions: processExpiredDeletions(
dataSources,
deletionConfig,
blobStorage,
),
};
};
```
2. Extract the default config constant
This keeps the factory easy to skim and makes defaults discoverable:
```ts
const DEFAULT_LISTING_DELETION_CONFIG: ListingDeletionConfig = {
archivalMonths: 6,
batchSize: 100,
blobContainerName: 'listing-images',
};
```
This approach preserves all existing behavior and call sites but confines the union-type complexity and defaulting logic to small, focused helpers.
</issue_to_address>
### Comment 6
<location> `packages/sthrift/application-services/src/contexts/listing/index.ts:20` </location>
<code_context>
+ listingDeletionConfig?: ListingDeletionConfig;
+}
+
export const Listing = (
- dataSources: DataSources,
+ deps: DataSources | ListingDependencies,
</code_context>
<issue_to_address>
**issue (complexity):** Consider reusing the existing dependency type and moving the union/normalization logic into a small helper to keep the Listing factory as straightforward wiring code.
You can keep the new feature and reduce complexity by (1) reusing the existing `ItemListingDependencies` type and (2) extracting the union handling into a small helper. That keeps `Listing` as a simple composition module and moves the adapter logic into a dedicated function.
For example:
```ts
// Reuse existing type instead of redefining a parallel one
type ListingDependencies = ItemListingDependencies;
type ListingInput = DataSources | ListingDependencies;
const normalizeListingDeps = (deps: ListingInput): ItemListingDependencies => {
if ('dataSources' in deps) {
return deps;
}
return { dataSources: deps };
};
export const Listing = (
deps: ListingInput,
): ListingContextApplicationService => {
const itemListingDeps = normalizeListingDeps(deps);
return {
ItemListing: ItemListingApi(itemListingDeps),
};
};
```
If you want to make the contract even clearer while still supporting legacy callers, you can add overloads without changing behavior:
```ts
export function Listing(
deps: DataSources,
): ListingContextApplicationService;
export function Listing(
deps: ListingDependencies,
): ListingContextApplicationService;
export function Listing(
deps: ListingInput,
): ListingContextApplicationService {
const itemListingDeps = normalizeListingDeps(deps);
return { ItemListing: ItemListingApi(itemListingDeps) };
}
```
This keeps all functionality, but the main factory now reads as a simple wiring function, and the type discrimination/adapter logic is isolated and testable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Implements automatic deletion of expired listings after 6 months archival period, following Azure Functions timer trigger pattern.
Closes #200
Changes
Timer Handler
expired-listing-deletion-handler.ts- Azure Functions timer that runs daily at 2 AM (NCRONTAB:0 0 2 * * *)Application Services
processExpiredDeletionsservice to batch delete expired/cancelled listings older than 6 monthsdeleteByListingservice to clean up related conversations when a listing is deletedforSystemTask()factory method for system-level operations without user context (e.g., timer-triggered tasks)Domain Layer
requestDelete()method to Conversation aggregate (following existing ItemListing pattern)deleteBlob()method to BlobStorage service interface for image cleanupPersistence Layer
getExpiredForDeletion(archivalMonths, limit)to ItemListingReadRepositorygetByListingId(listingId)to ConversationReadRepositoryInfrastructure
@azure/storage-blobto service-blob-storage for blob cleanup operations@sthrift/domaindependency to context-spec packageTesting
How to Test
stateto 'Expired' andupdatedAtto more than 6 months agoArchitecture
Summary by Sourcery
Add scheduled processing and permanent cleanup of expired listings, including conversations and images, wired into the Azure Functions host.
New Features:
Enhancements:
Build:
Tests: