Open
Conversation
matheusgondra
requested review from
Yuri-amaralsantos,
andersonnaz and
Copilot
There was a problem hiding this comment.
Pull request overview
This PR refactors the codebase to use a standardized result/response type system by introducing ResultResponse<T> and PageResult<T, K> discriminated union types. This improves type safety by clearly distinguishing between success and failure cases using TypeScript's type narrowing capabilities.
Changes:
- Introduced new type definitions:
ResultResponse<T>(discriminated union for success/failure) andPageResult<T, K>(paginated result wrapper) - Updated all use-case type definitions to use the new result types, replacing the previous flexible object structures
- Refactored implementations and controllers to remove unnecessary type assertions (e.g.,
as Error) that are no longer needed with the discriminated union - Updated test stubs and test cases to align with the new type structure, including proper success response data
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/domain/types/result.ts | New discriminated union type ResultResponse<T> with Success and Failure variants |
| src/domain/types/page-result.ts | New paginated result type PageResult<T, K> built on top of ResultResponse |
| src/domain/use-cases/tasks/*.ts | Updated task-related use cases to use ResultResponse and PageResult types |
| src/domain/use-cases/scheduler/**/*.ts | Updated scheduler and tag use cases to use ResultResponse types |
| src/domain/use-cases/pet/*.ts | Updated pet use cases to use ResultResponse types |
| src/domain/use-cases/settings/*.ts | Updated settings use cases to use ResultResponse types |
| src/data/protocols/service/events-generator.ts | Updated EventsGenerator protocol to use ResultResponse |
| src/infra/service/events-generator-service.ts | Updated service implementation to return proper success data and removed data from error responses |
| src/data/use-cases/scheduler/tag/db-delete-tag-by-id.ts | Added data: undefined to success response to satisfy ResultResponse type |
| src/data/use-cases/pet/db-update-pet.ts | Removed type assertions by leveraging discriminated union type narrowing |
| src/data/use-cases/pet/db-appoint-pet.ts | Refined internal result types as discriminated unions |
| src/data/use-cases/pet/db-add-pet.ts | Removed type assertions by leveraging discriminated union type narrowing |
| src/application/controllers/**/*.ts | Removed as Error type assertions now guaranteed by the discriminated union |
| tests/utils/stubs/use-case.stub.ts | Updated test stubs to match new type structure (inline return, added data field) |
| tests/src/main/routes/tasks-routes.test.ts | Removed debug console.log statement |
| tests/src/infra/service/events-generator-service.spec.ts | Updated test expectations to verify success data structure and removed data from error expectations |
Comments suppressed due to low confidence (3)
src/application/controllers/scheduler/create-scheduler.ts:50
- With the new ResultResponse type system, when result.isSuccess is true, result.data is guaranteed to be defined. Using optional chaining (?.) undermines this type safety. Remove the optional chaining and access all result.data properties directly (result.data.id, result.data.guardianId, etc.).
id: result.data?.id,
guardianId: result.data?.guardianId,
tagId: result.data?.tagId,
title: result.data?.title,
description: result.data?.description,
note: result.data?.note,
startAt: result.data?.startAt,
endAt: result.data?.endAt,
daysOfWeek: result.data?.daysOfWeek,
daysOfMonth: result.data?.daysOfMonth,
daily: result.data?.daily,
pets: result.data?.pets
src/application/controllers/settings/update-settings.ts:29
- With the new ResultResponse type system, when result.isSuccess is true, TypeScript's type narrowing guarantees that result.data is defined (not undefined). Using optional chaining (?.) here undermines this type safety and could mask potential issues. Change result.data?.guardianId to result.data.guardianId, result.data?.notificationEmail to result.data.notificationEmail, and result.data?.notificationMobile to result.data.notificationMobile.
guardianId: result.data?.guardianId,
notificationEmail: result.data?.notificationEmail,
notificationMobile: result.data?.notificationMobile
src/application/controllers/scheduler/tag/add-tag.ts:30
- With the new ResultResponse type system, when result.isSuccess is true, TypeScript's type narrowing guarantees that result.data is defined. Using optional chaining (?.) here undermines this type safety. Remove the optional chaining and access result.data.id, result.data.guardianId, result.data.name, and result.data.color directly.
id: result.data?.id,
guardianId: result.data?.guardianId,
name: result.data?.name,
color: result.data?.color
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
|
|
||
| it('Should return NotAcceptableError if loadByDate returns null because date conflits', async () => { | ||
| it('Should return NotAcceptableError if loadByDate not returns null because date conflits', async () => { |
There was a problem hiding this comment.
Spelling error: 'conflits' should be 'conflicts'.
| }) | ||
|
|
||
| it('Should return NotAcceptableError if loadByDate returns null because date conflits', async () => { | ||
| it('Should return NotAcceptableError if loadByDate not returns null because date conflits', async () => { |
There was a problem hiding this comment.
Spelling error: 'conflits' should be 'conflicts'.
| }) | ||
|
|
||
| it('Should return NotAcceptableError if loadByDate returns null because date conflits', async () => { | ||
| it('Should return NotAcceptableError if loadByDate not returns null because date conflits', async () => { |
There was a problem hiding this comment.
Spelling error: 'conflits' should be 'conflicts'.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Falta resolver os conflitos e resolver esses erros do copilot |
andersonnaz
requested changes
Feb 5, 2026
| type Success<T> = { | ||
| isSuccess: true | ||
| data: T | ||
| error?: never |
There was a problem hiding this comment.
talvez remover o error do type success seja melhor
| type Failure = { | ||
| isSuccess: false | ||
| error: Error | ||
| data?: never |
There was a problem hiding this comment.
talvez remover o data do type Failure seja melhor.
| if (daysOfWeek.includes(currentDate.getDay())) { | ||
| const event = await this.eventRepository.loadByDate({ date: currentDate }) | ||
| if (event) { | ||
| const eventAlready = await this.eventRepository.loadByDate({ date: new Date(currentDate) }) |
There was a problem hiding this comment.
por que do new date aqui se currentDate já é um objeto Date?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
No description provided.