[feat] #36 - 타이머완료/종료시 ai 피드백 생성#83
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (20)
WalkthroughAI 투두 컨트롤러의 라우팅 경로가 변경되고, 투두 완료/피드백 생성을 위한 새로운 DTO(TodoFeedbackSource, GeminiTodoFeedback, TodoDurationHistoryRow), 프롬프트 빌더(TodoFeedbackPromptBuilder), AiTodoService.createFeedback 메서드가 추가됩니다. 히스토리 조회가 LocalDate 기반에서 사용자 타임존(ZoneId) 기반으로 전환되고, TimerController/TimerService/TimerRecord가 타이머 완료/중단 시 AI 피드백을 생성·저장하도록 확장됩니다. 예외 처리는 CustomException/AiErrorCode 기반으로 통일됩니다. ChangesAI 피드백 생성 및 타임존 기반 히스토리 조회
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aneykrap
force-pushed
the
feat/#36-ai-feedback
branch
from
12afbc2 to
6dcd3ba
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/Timo/Timo/domain/ai/prompt/TodoFeedbackPromptBuilder.java (1)
71-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win중복된 프롬프트 포맷 헬퍼를 분리하세요
TodoFeedbackPromptBuilder와TodoDurationPromptBuilder에escapeJsonString,toMinutes,formatHistories가 같은 형태로 중복되어 있습니다. 공통 헬퍼로 빼면 JSON 이스케이프와 분 단위 변환 규칙을 한 곳에서 유지할 수 있어 출력 불일치 위험이 줄어듭니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/Timo/Timo/domain/ai/prompt/TodoFeedbackPromptBuilder.java` around lines 71 - 105, The prompt formatting helpers are duplicated across TodoFeedbackPromptBuilder and TodoDurationPromptBuilder; extract the shared logic for formatHistories, toMinutes, and escapeJsonString into a common helper or base class, then update TodoFeedbackPromptBuilder to delegate to that shared implementation so JSON escaping and minute conversion stay consistent in one place.Source: Path instructions
src/main/java/com/Timo/Timo/domain/timer/entity/TimerRecord.java (1)
98-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
finish()메서드에targetStatus도메인 규칙 검증이 누락되었습니다.
finish()는targetStatus로 어떤TimerStatus값이든 받을 수 있습니다.COMPLETED또는STOPPED만 허용하도록 검증을 추가하여 엔티티 수준에서 상태 전환 규칙을 캡슐화해야 합니다. 현재 호출부(TimerService.finishTimer)에서만 올바른 값을 전달하고 있으므로 즉각적 장애는 아니지만, 엔티티 불변식(immutable) 보장 차원에서 보완이 필요합니다.As per coding guidelines, Entity에 필요한 도메인 규칙과 상태 변경 메서드가 적절히 캡슐화되어 있는지 확인해 주세요.
♻️ 제안하는 검증 로직 추가
public void finish(TimerStatus targetStatus, LocalDateTime endedAt, int actualSeconds) { if (isFinished()) { throw new CustomException(TimerErrorCode.TIMER_ALREADY_FINISHED); } + if (targetStatus != TimerStatus.COMPLETED && targetStatus != TimerStatus.STOPPED) { + throw new CustomException(TimerErrorCode.TIMER_INVALID_STATUS_TRANSITION); + } this.status = targetStatus; this.endedAt = endedAt; this.actualSeconds = actualSeconds; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/Timo/Timo/domain/timer/entity/TimerRecord.java` around lines 98 - 105, `TimerRecord.finish()`에서 `targetStatus`에 대한 도메인 규칙 검증이 빠져 있습니다. `finish()` 내부에서 `TimerStatus`가 `COMPLETED` 또는 `STOPPED`일 때만 상태 변경을 허용하도록 검증을 추가하고, 그 외 값은 `CustomException`으로 거부해 엔티티 수준의 상태 전환 규칙을 캡슐화하세요. 이 변경은 `TimerRecord.finish` 메서드에 반영하고, 현재 호출부인 `TimerService.finishTimer`에만 의존하지 않도록 보완하면 됩니다.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/Timo/Timo/domain/ai/service/AiTodoService.java`:
- Around line 85-92: In createFeedback’s history lookup inside AiTodoService,
the current todo’s just-finished timer record can still be included because the
query only uses toExclusive; update the history fetch so it excludes the active
todo identity as well. Use the existing findHistories call in createFeedback and
pass/filter by the current todoId (or the corresponding timer record identifier)
so feedback history cannot surface the same TODO’s own recent record, even when
the title matches exactly.
In `@src/main/java/com/Timo/Timo/domain/timer/service/TimerService.java`:
- Around line 115-116: The `finishTimer` flow in `TimerService` is holding the
pessimistic lock too long because `generateAiFeedback` leads to the external
Gemini call inside the same `@Transactional` boundary. After
`timerRecordRepository.flush()`, move AI feedback generation out of the current
transaction by calling a separate method/service with `REQUIRES_NEW` or
scheduling it after commit, and ensure any downstream write path such as
`generateAiFeedback`/`aiTodoService.createFeedback`/`updateAiFeedback` also runs
in its own transaction.
---
Nitpick comments:
In `@src/main/java/com/Timo/Timo/domain/ai/prompt/TodoFeedbackPromptBuilder.java`:
- Around line 71-105: The prompt formatting helpers are duplicated across
TodoFeedbackPromptBuilder and TodoDurationPromptBuilder; extract the shared
logic for formatHistories, toMinutes, and escapeJsonString into a common helper
or base class, then update TodoFeedbackPromptBuilder to delegate to that shared
implementation so JSON escaping and minute conversion stay consistent in one
place.
In `@src/main/java/com/Timo/Timo/domain/timer/entity/TimerRecord.java`:
- Around line 98-105: `TimerRecord.finish()`에서 `targetStatus`에 대한 도메인 규칙 검증이 빠져
있습니다. `finish()` 내부에서 `TimerStatus`가 `COMPLETED` 또는 `STOPPED`일 때만 상태 변경을 허용하도록
검증을 추가하고, 그 외 값은 `CustomException`으로 거부해 엔티티 수준의 상태 전환 규칙을 캡슐화하세요. 이 변경은
`TimerRecord.finish` 메서드에 반영하고, 현재 호출부인 `TimerService.finishTimer`에만 의존하지 않도록
보완하면 됩니다.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5a3a052-e24d-4c0e-81fe-dd498b2ee9dd
⛔ Files ignored due to path filters (3)
src/main/java/com/Timo/Timo/domain/ai/docs/AiTodoDocs.javais excluded by!**/docs/**src/main/java/com/Timo/Timo/domain/timer/docs/TimerCompleteControllerDocs.javais excluded by!**/docs/**src/main/java/com/Timo/Timo/domain/timer/docs/TimerStopControllerDocs.javais excluded by!**/docs/**
📒 Files selected for processing (25)
src/main/java/com/Timo/Timo/domain/ai/controller/AiTodoController.javasrc/main/java/com/Timo/Timo/domain/ai/dto/TodoDurationHistory.javasrc/main/java/com/Timo/Timo/domain/ai/dto/TodoFeedbackSource.javasrc/main/java/com/Timo/Timo/domain/ai/dto/request/RecommendDurationRequest.javasrc/main/java/com/Timo/Timo/domain/ai/dto/response/GeminiTodoFeedback.javasrc/main/java/com/Timo/Timo/domain/ai/exception/AiErrorCode.javasrc/main/java/com/Timo/Timo/domain/ai/prompt/TodoDurationPromptBuilder.javasrc/main/java/com/Timo/Timo/domain/ai/prompt/TodoFeedbackPromptBuilder.javasrc/main/java/com/Timo/Timo/domain/ai/repository/AiTodoQueryRepository.javasrc/main/java/com/Timo/Timo/domain/ai/service/AiRequestRateLimiter.javasrc/main/java/com/Timo/Timo/domain/ai/service/AiTodoHistories.javasrc/main/java/com/Timo/Timo/domain/ai/service/AiTodoHistoryService.javasrc/main/java/com/Timo/Timo/domain/ai/service/AiTodoService.javasrc/main/java/com/Timo/Timo/domain/ai/service/GeminiService.javasrc/main/java/com/Timo/Timo/domain/tag/entity/Tag.javasrc/main/java/com/Timo/Timo/domain/timer/controller/TimerController.javasrc/main/java/com/Timo/Timo/domain/timer/dto/response/TimerFinishResponse.javasrc/main/java/com/Timo/Timo/domain/timer/entity/TimerRecord.javasrc/main/java/com/Timo/Timo/domain/timer/exception/TimerErrorCode.javasrc/main/java/com/Timo/Timo/domain/timer/exception/TimerSuccessCode.javasrc/main/java/com/Timo/Timo/domain/timer/factory/TimerResponseFactory.javasrc/main/java/com/Timo/Timo/domain/timer/repository/TimerRecordRepository.javasrc/main/java/com/Timo/Timo/domain/timer/repository/TimerSessionRepository.javasrc/main/java/com/Timo/Timo/domain/timer/service/TimerService.javasrc/main/java/com/Timo/Timo/domain/todo/exception/TodoErrorCode.java
💤 Files with no reviewable changes (1)
- src/main/java/com/Timo/Timo/domain/timer/factory/TimerResponseFactory.java
laura-jung
approved these changes
Jul 9, 2026
laura-jung
left a comment
laura-jung
left a comment
Collaborator
There was a problem hiding this comment.
아주 굳입니다아아ㅏ아아ㅏ
프롬포트도 깔꼼해요
궁금한점만 남겨두었으니 확인부탁드려요
|
|
||
| @Override | ||
| @PostMapping("/recommend-duration") | ||
| @PostMapping("/ai/duration") |
Collaborator
There was a problem hiding this comment.
p3) 도메인이 ai라 이후 ai 관련 api가 추가되면 url에서 ai가 반복될 것 같아요! requestMapping에 ai 까지 넣는게 좋을 것 같습니다
Collaborator
Author
There was a problem hiding this comment.
@RequestMapping("/api/v1/ai")@PostMapping("/duration")으로 반영했습니다
Comment on lines
+54
to
+61
| Object[] row = (Object[])rows.get(0); | ||
| return new TodoFeedbackSource( | ||
| (String)row[0], | ||
| toLong(row[1]), | ||
| (String)row[2], | ||
| toInteger(row[3]), | ||
| toInteger(row[4]) | ||
| ); |
Collaborator
Author
There was a problem hiding this comment.
AI 피드백 생성을 위해 현재 투두 정보를 native query로 조회한 뒤 결과를 Object[]에서 컬럼 순서대로 꺼내 TodoFeedbackSource로 변환하는 코드입니다.
row[0]: 투두명row[1]: 태그 IDrow[2]: 태그명row[3]: 예상 소요 시간row[4]: 최근 실제 소요 시간
이것도 다시 생각해보니 컬럼 순서에 의존하기 때문에 유지보수성과 타입 안정성이 떨어질 수 있다고 생각해서 native query와 Object[] 매핑을 제거하고 JPQL constructor projection으로 리팩토링했습니다
Jy000n
approved these changes
Jul 9, 2026
Jy000n
left a comment
Jy000n
left a comment
Collaborator
There was a problem hiding this comment.
고생하셨습니다아
한 가지 궁금한 게 있었는데요, 전체적으로 AI 에러 코드가 도메인 에러 코드를 만들어서 처리하지 않고, IIllegalStateException, IllegalArgumntException으로 처리하신 것 같은데 이유가 따로 있으실까요 ??
Collaborator
Author
현재 해당 예외들은 사용자 요청값 오류보다는 Gemini 응답 파싱 실패나 필수 필드 누락처럼 서버 내부 처리 과정에서 발생하는 예외라고 판단해 말씀해주신 것처럼 AI 도메인 예외로 분리하는 편이 더 명확할 것 같아 |
aneykrap
force-pushed
the
feat/#36-ai-feedback
branch
from
6dcd3ba to
bc3fc30
Compare
aneykrap
force-pushed
the
feat/#36-ai-feedback
branch
from
bc3fc30 to
91cd60f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/com/Timo/Timo/domain/ai/service/AiTodoService.java (1)
163-171: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAI 피드백 길이 제한을 서버에서도 검증해 주세요.
PR 목표는 1–2문장 피드백인데, 현재는 blank만 검증합니다. 모델이 긴 응답을 반환하면 저장 컬럼/UI 계약을 넘길 수 있으니 최소한 최대 길이 guard를 두는 편이 안전합니다.
제안 수정
private static final int ESTIMATED_RESPONSE_TOKEN_COST = 200; private static final int TOKEN_ESTIMATE_CHAR_DIVISOR = 4; + private static final int MAX_FEEDBACK_LENGTH = 300; @@ - return feedback.feedback().trim(); + String trimmed = feedback.feedback().trim(); + if (trimmed.length() > MAX_FEEDBACK_LENGTH) { + throw new CustomException(AiErrorCode.AI_INVALID_RESPONSE); + } + return trimmed; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/Timo/Timo/domain/ai/service/AiTodoService.java` around lines 163 - 171, The validateFeedback method in AiTodoService currently only rejects null or blank GeminiTodoFeedback content, so add a server-side maximum length guard before returning the trimmed feedback. Update the validation logic in validateFeedback to enforce the PR’s intended short feedback contract (1–2 sentences) by checking feedback.feedback().length() against a safe limit and throwing CustomException with AiErrorCode.AI_INVALID_RESPONSE when exceeded. Keep the trimming behavior after validation so the stored value remains normalized.src/main/java/com/Timo/Timo/domain/ai/exception/AiErrorCode.java (1)
14-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAI 500 계열 에러 code를 구분해 주세요.
세 오류가 모두
"AI_500"이면AiErrorCode로 분리한 이점이 응답/모니터링 계약에 반영되지 않습니다. 원인별로 고유 code를 주는 편이 추적하기 쉽습니다.제안 수정
- AI_CONFIG_NOT_FOUND(HttpStatus.INTERNAL_SERVER_ERROR, "AI_500", "AI 설정 정보가 올바르지 않습니다."), - AI_RESPONSE_PARSE_FAILED(HttpStatus.INTERNAL_SERVER_ERROR, "AI_500", "AI 응답을 처리하는 중 오류가 발생했습니다."), - AI_INVALID_RESPONSE(HttpStatus.INTERNAL_SERVER_ERROR, "AI_500", "AI 응답 형식이 올바르지 않습니다."); + AI_CONFIG_NOT_FOUND(HttpStatus.INTERNAL_SERVER_ERROR, "AI_CONFIG_NOT_FOUND", "AI 설정 정보가 올바르지 않습니다."), + AI_RESPONSE_PARSE_FAILED(HttpStatus.INTERNAL_SERVER_ERROR, "AI_RESPONSE_PARSE_FAILED", "AI 응답을 처리하는 중 오류가 발생했습니다."), + AI_INVALID_RESPONSE(HttpStatus.INTERNAL_SERVER_ERROR, "AI_INVALID_RESPONSE", "AI 응답 형식이 올바르지 않습니다.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/Timo/Timo/domain/ai/exception/AiErrorCode.java` around lines 14 - 17, The AI 500-class errors in AiErrorCode are currently sharing the same code, so update the enum entries for AI_CONFIG_NOT_FOUND, AI_RESPONSE_PARSE_FAILED, and AI_INVALID_RESPONSE to use distinct error codes instead of a common "AI_500" value. Keep AI_RATE_LIMIT_EXCEEDED as-is, and assign each internal-server case a unique identifier so callers, logs, and monitoring can distinguish the failure source through AiErrorCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/Timo/Timo/domain/timer/entity/TimerRecord.java`:
- Around line 98-105: The TimerRecord.finish method currently accepts any
TimerStatus, which allows non-terminal states to be persisted with endedAt and
actualSeconds set. Update finish in TimerRecord to enforce the domain rule by
validating targetStatus and only allowing terminal statuses such as COMPLETED
and STOPPED; reject RUNNING/PAUSED with a CustomException or equivalent. Keep
the state transition logic encapsulated inside TimerRecord so callers cannot
bypass the rule.
In `@src/main/java/com/Timo/Timo/domain/timer/service/TimerService.java`:
- Around line 155-173: `TimerService.finishTimer` still runs inside the
class-level `@Transactional(readOnly = true)`, so the new `TransactionTemplate`
can join the existing transaction and keep the `findByIdForUpdate` lock held
during `generateAiFeedback`. Update the transaction boundary so the status
update in `finishTimerInTransaction` runs in a separate write transaction from
the AI call, using `TransactionTemplate` with a propagation mode that always
starts a new transaction (or otherwise ensure the write block does not join the
outer one). Keep `completeTimer`/`stopTimer` as thin wrappers and make sure
`updateAiFeedback` also runs outside the lock-holding transaction.
- Around line 202-204: In TimerService’s stop/finish handling, the current flow
always calls TodoInstance.markCompleted() after stopTimer(), which incorrectly
marks STOPPED timers as completed. Update the logic around
getOrCreateInstance(...) so completion is applied only for the true completed
path, and keep STOPPED handling limited to stopTimer(); use TimerStatus to
branch the behavior clearly and preserve separate states for STOPPED vs
completed.
---
Nitpick comments:
In `@src/main/java/com/Timo/Timo/domain/ai/exception/AiErrorCode.java`:
- Around line 14-17: The AI 500-class errors in AiErrorCode are currently
sharing the same code, so update the enum entries for AI_CONFIG_NOT_FOUND,
AI_RESPONSE_PARSE_FAILED, and AI_INVALID_RESPONSE to use distinct error codes
instead of a common "AI_500" value. Keep AI_RATE_LIMIT_EXCEEDED as-is, and
assign each internal-server case a unique identifier so callers, logs, and
monitoring can distinguish the failure source through AiErrorCode.
In `@src/main/java/com/Timo/Timo/domain/ai/service/AiTodoService.java`:
- Around line 163-171: The validateFeedback method in AiTodoService currently
only rejects null or blank GeminiTodoFeedback content, so add a server-side
maximum length guard before returning the trimmed feedback. Update the
validation logic in validateFeedback to enforce the PR’s intended short feedback
contract (1–2 sentences) by checking feedback.feedback().length() against a safe
limit and throwing CustomException with AiErrorCode.AI_INVALID_RESPONSE when
exceeded. Keep the trimming behavior after validation so the stored value
remains normalized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 78052216-48e4-4f75-8e85-d2c0cb290c66
⛔ Files ignored due to path filters (3)
src/main/java/com/Timo/Timo/domain/ai/docs/AiTodoDocs.javais excluded by!**/docs/**src/main/java/com/Timo/Timo/domain/timer/docs/TimerCompleteControllerDocs.javais excluded by!**/docs/**src/main/java/com/Timo/Timo/domain/timer/docs/TimerStopControllerDocs.javais excluded by!**/docs/**
📒 Files selected for processing (21)
src/main/java/com/Timo/Timo/domain/ai/controller/AiTodoController.javasrc/main/java/com/Timo/Timo/domain/ai/dto/TodoDurationHistory.javasrc/main/java/com/Timo/Timo/domain/ai/dto/TodoFeedbackSource.javasrc/main/java/com/Timo/Timo/domain/ai/dto/request/RecommendDurationRequest.javasrc/main/java/com/Timo/Timo/domain/ai/dto/response/GeminiTodoFeedback.javasrc/main/java/com/Timo/Timo/domain/ai/exception/AiErrorCode.javasrc/main/java/com/Timo/Timo/domain/ai/prompt/TodoDurationPromptBuilder.javasrc/main/java/com/Timo/Timo/domain/ai/prompt/TodoFeedbackPromptBuilder.javasrc/main/java/com/Timo/Timo/domain/ai/repository/AiTodoQueryRepository.javasrc/main/java/com/Timo/Timo/domain/ai/repository/TodoDurationHistoryRow.javasrc/main/java/com/Timo/Timo/domain/ai/service/AiRequestRateLimiter.javasrc/main/java/com/Timo/Timo/domain/ai/service/AiTodoHistories.javasrc/main/java/com/Timo/Timo/domain/ai/service/AiTodoHistoryService.javasrc/main/java/com/Timo/Timo/domain/ai/service/AiTodoService.javasrc/main/java/com/Timo/Timo/domain/ai/service/GeminiService.javasrc/main/java/com/Timo/Timo/domain/tag/entity/Tag.javasrc/main/java/com/Timo/Timo/domain/timer/controller/TimerController.javasrc/main/java/com/Timo/Timo/domain/timer/dto/response/TimerFinishResponse.javasrc/main/java/com/Timo/Timo/domain/timer/entity/TimerRecord.javasrc/main/java/com/Timo/Timo/domain/timer/service/TimerService.javasrc/main/java/com/Timo/Timo/domain/todo/exception/TodoErrorCode.java
✅ Files skipped from review due to trivial changes (7)
- src/main/java/com/Timo/Timo/domain/ai/dto/TodoDurationHistory.java
- src/main/java/com/Timo/Timo/domain/ai/service/AiRequestRateLimiter.java
- src/main/java/com/Timo/Timo/domain/tag/entity/Tag.java
- src/main/java/com/Timo/Timo/domain/ai/service/AiTodoHistories.java
- src/main/java/com/Timo/Timo/domain/ai/dto/request/RecommendDurationRequest.java
- src/main/java/com/Timo/Timo/domain/ai/prompt/TodoDurationPromptBuilder.java
- src/main/java/com/Timo/Timo/domain/todo/exception/TodoErrorCode.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/java/com/Timo/Timo/domain/ai/dto/response/GeminiTodoFeedback.java
- src/main/java/com/Timo/Timo/domain/ai/dto/TodoFeedbackSource.java
- src/main/java/com/Timo/Timo/domain/ai/service/GeminiService.java
- src/main/java/com/Timo/Timo/domain/ai/prompt/TodoFeedbackPromptBuilder.java
- src/main/java/com/Timo/Timo/domain/ai/service/AiTodoHistoryService.java
- src/main/java/com/Timo/Timo/domain/timer/controller/TimerController.java
Comment on lines
+98
to
105
| public void finish(TimerStatus targetStatus, LocalDateTime endedAt, int actualSeconds) { | ||
| if (isFinished()) { | ||
| throw new CustomException(TimerErrorCode.TIMER_ALREADY_FINISHED); | ||
| } | ||
| this.status = targetStatus; | ||
| this.endedAt = endedAt; | ||
| this.actualSeconds = actualSeconds; | ||
| this.aiFeedback = aiFeedback; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
finish에서 종료 상태만 허용하도록 검증해 주세요.
현재 public 메서드가 TimerStatus를 그대로 받아 RUNNING/PAUSED도 endedAt, actualSeconds가 채워진 상태로 만들 수 있습니다. 엔티티 내부에서 COMPLETED/STOPPED만 허용하도록 막는 편이 안전합니다.
수정 예시
public void finish(TimerStatus targetStatus, LocalDateTime endedAt, int actualSeconds) {
if (isFinished()) {
throw new CustomException(TimerErrorCode.TIMER_ALREADY_FINISHED);
}
+ if (targetStatus != TimerStatus.COMPLETED && targetStatus != TimerStatus.STOPPED) {
+ throw new CustomException(TimerErrorCode.TIMER_INVALID_STATUS_TRANSITION);
+ }
this.status = targetStatus;
this.endedAt = endedAt;
this.actualSeconds = actualSeconds;
}As per path instructions, "Entity에 필요한 도메인 규칙과 상태 변경 메서드가 적절히 캡슐화되어 있는지 확인해 주세요."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/Timo/Timo/domain/timer/entity/TimerRecord.java` around
lines 98 - 105, The TimerRecord.finish method currently accepts any TimerStatus,
which allows non-terminal states to be persisted with endedAt and actualSeconds
set. Update finish in TimerRecord to enforce the domain rule by validating
targetStatus and only allowing terminal statuses such as COMPLETED and STOPPED;
reject RUNNING/PAUSED with a CustomException or equivalent. Keep the state
transition logic encapsulated inside TimerRecord so callers cannot bypass the
rule.
Source: Path instructions
Comment on lines
202
to
204
| TodoInstance instance = getOrCreateInstance(timerRecord.getTodo(), timerRecord.getStartedAt().toLocalDate()); | ||
| instance.stopTimer(); | ||
| instance.markCompleted(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
중단(STOPPED) 시에도 Todo가 완료 처리됩니다.
stopTimer도 TimerStatus.STOPPED로 이 경로를 타는데, Line 204에서 항상 markCompleted()를 호출합니다. 중단과 완료 상태를 분리하려면 완료 상태일 때만 완료 처리해야 합니다.
수정 예시
TodoInstance instance = getOrCreateInstance(timerRecord.getTodo(), timerRecord.getStartedAt().toLocalDate());
instance.stopTimer();
-instance.markCompleted();
+if (targetStatus == TimerStatus.COMPLETED) {
+ instance.markCompleted();
+}As per path instructions, "비즈니스 로직이 명확하고 테스트 가능하게 작성되어 있는지 확인해 주세요."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TodoInstance instance = getOrCreateInstance(timerRecord.getTodo(), timerRecord.getStartedAt().toLocalDate()); | |
| instance.stopTimer(); | |
| instance.markCompleted(); | |
| TodoInstance instance = getOrCreateInstance(timerRecord.getTodo(), timerRecord.getStartedAt().toLocalDate()); | |
| instance.stopTimer(); | |
| if (targetStatus == TimerStatus.COMPLETED) { | |
| instance.markCompleted(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/Timo/Timo/domain/timer/service/TimerService.java` around
lines 202 - 204, In TimerService’s stop/finish handling, the current flow always
calls TodoInstance.markCompleted() after stopTimer(), which incorrectly marks
STOPPED timers as completed. Update the logic around getOrCreateInstance(...) so
completion is applied only for the true completed path, and keep STOPPED
handling limited to stopTimer(); use TimerStatus to branch the behavior clearly
and preserve separate states for STOPPED vs completed.
Source: Path instructions
laura-jung
approved these changes
Jul 9, 2026
laura-jung
left a comment
laura-jung
left a comment
Collaborator
There was a problem hiding this comment.
코드가 좀더 깔끔해진 것 같네요! 수고하셨습니다.
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.
관련 이슈 🛠
작업 내용 요약 ✏️
타이머 종료 및 완료 시 예상 시간과 실제 수행 시간을 비교하고 과거 수행 기록을 참고해 AI 피드백을 생성하는 기능을 구현했습니다.
생성된 피드백은 타이머 기록에 저장되며 종료·완료 API의
aiFeedback필드로 반환됩니다.주요 변경 사항 🛠️
PATCH /api/v1/timers/{timerId}/stop응답에 AI 피드백 연동PATCH /api/v1/timers/{timerId}/complete응답에 AI 피드백 연동유사한 제목의 과거 기록과 같은 태그의 최근 기록을 Gemini에 전달
현재 결과 관찰, 과거 패턴 해석, 다음 행동 추천을 1~2문장으로 생성
생성된 피드백을
timer_records.ai_feedback에 저장사용자의 저장된
zoneId를 기준으로 과거 기록 조회 날짜 계산AI 예상 소요 시간 API를
POST /api/v1/ai/duration으로 변경트러블 슈팅 ⚽️
타이머 종료 직후 AI 피드백을 생성할 경우 영속성 컨텍스트의 변경 사항이 아직 DB에 반영되지 않아 AI 조회 쿼리가 방금 계산한
actualSeconds를 조회하지 못하는 경우가 있다고 판단했어요.타이머 상태, 종료 시각, 실제 수행 시간을 반영한 뒤
flush()를 실행하고 AI 피드백을 생성하도록 처리 순서를 변경했습니다. 이를 통해 종료된 타이머의 최신 실제 수행 시간이 Gemini에 전달되고 생성된 피드백이 종료·완료 응답에 포함되도록 했습니다.테스트 결과 📄
타이머 종료/ 완료 시점에서 ai 피드백 응답 확인
스크린샷 📷
리뷰 요구사항 📢
AI 피드백 생성은 타이머 종료 트랜잭션 안에서 수행하지만 Gemini 호출 실패가 타이머 종료 처리까지 롤백시키지 않도록 예외를 분리했습니다.
AI 호출에 실패하면 타이머 상태, 종료 시각, 실제 수행 시간과 투두 완료 상태는 정상 저장되며
aiFeedback은null로 반환됩니다. AI 피드백을 부가 기능으로 처리하는 정책이 적절한 지 잘 모르겠어요 여러의견 주시면 감사하겟습니다📎 참고 자료 (선택)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor