Merged
Conversation
8 tasks
📝 WalkthroughWalkthroughThe PR replaces range-based plans (start_date + total_days) with explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as PlansAPI
participant DB as Database
Client->>API: POST /plans (training_dates[])
API->>DB: INSERT SavedPlan {training_dates: [...], total_days: len(...)}
DB-->>API: Insert OK / SavedPlan ID
API-->>Client: 201 Created (SavedPlanResponse)
Client->>API: GET /plans?year=YYYY&month=MM
API->>DB: SELECT * FROM saved_plans WHERE user_id=...
DB-->>API: [SavedPlan{training_dates:[...]}...]
API->>API: filter plans where any training_dates entry startsWith "YYYY-MM"
API-->>Client: 200 OK (filtered plans)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![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/api/v1/endpoints/plans.py (2)
26-27: Avoid persisting a second source of truth for plan length.
total_daysis derived fromtraining_dates, but it is still stored separately. If those ever drift, this endpoint will return a different length thancomplete_plan_dayenforces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v1/endpoints/plans.py` around lines 26 - 27, The code is persisting total_days separately while it is derivable from training_dates (training_dates and total_days in the plan creation), creating a second source of truth; remove the total_days field from the persisted payload and any creation logic (where total_days is set) and instead compute plan length on demand from len(plan.training_dates) (or provide a property/method like complete_plan_day/plan_length that returns len(training_dates)) so stored data cannot drift from the derived value.
43-54: This monthly query now scans the user's full plan history in Python.That will get slower as saved plans grow because the database only filters by
user_id. Consider pushing the month predicate into SQL, or normalizingtraining_datesinto a child table so calendar reads stay queryable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/v1/endpoints/plans.py` around lines 43 - 54, The current code loads all SavedPlan rows for a user into Python and then filters training_dates, which will scale poorly; modify the query that builds plans (the db.query(...) for SavedPlan) to include the month predicate in SQL so only matching rows are returned (e.g. add an additional filter that checks SavedPlan.training_dates for any element starting with month_prefix using a database-side JSON/array/text operator or a SQL func like jsonb_array_elements_text/LIKE), and keep the rest of the logic (building result with SavedPlanResponse.model_validate) the same; for long-term scalability consider normalizing the training_dates array into a child table (e.g., a PlanTrainingDate model) and query that table with a JOIN or EXISTS instead of scanning SavedPlan in Python.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.py`:
- Around line 43-52: The startup code currently unconditionally drops the
saved_plans table when it sees the legacy "start_date" column (inspect,
inspector, get_columns, start_date, conn.execute(text("DROP TABLE
saved_plans"))); instead stop deleting data and either fail fast or perform an
in-place migration/backfill. Replace the DROP TABLE branch with a call to a new
backfill_saved_plans(engine) utility (or raise a clear RuntimeError) that 1)
adds the new training_dates column if needed (ALTER TABLE), 2) iterates existing
saved_plans rows and derives training_dates from the legacy fields, and 3)
updates rows within a transaction; ensure to log progress (logger.info/error)
and only drop legacy columns after a successful backfill.
In `@src/models/plan_schema.py`:
- Line 13: The PlanSchema's training_dates field may contain duplicates or be
out-of-order which breaks day_number semantics; add a Pydantic validator (e.g.,
`@root_validator` or `@field_validator`) on PlanSchema that normalizes
training_dates by converting inputs to date objects, removing duplicates,
sorting ascending, and ensuring min_length>=1, and if normalization changes the
list update any dependent properties (like day_number calculation) or raise
ValidationError if invalid; reference the training_dates field and PlanSchema
class so the validator is applied before persisting.
---
Nitpick comments:
In `@src/api/v1/endpoints/plans.py`:
- Around line 26-27: The code is persisting total_days separately while it is
derivable from training_dates (training_dates and total_days in the plan
creation), creating a second source of truth; remove the total_days field from
the persisted payload and any creation logic (where total_days is set) and
instead compute plan length on demand from len(plan.training_dates) (or provide
a property/method like complete_plan_day/plan_length that returns
len(training_dates)) so stored data cannot drift from the derived value.
- Around line 43-54: The current code loads all SavedPlan rows for a user into
Python and then filters training_dates, which will scale poorly; modify the
query that builds plans (the db.query(...) for SavedPlan) to include the month
predicate in SQL so only matching rows are returned (e.g. add an additional
filter that checks SavedPlan.training_dates for any element starting with
month_prefix using a database-side JSON/array/text operator or a SQL func like
jsonb_array_elements_text/LIKE), and keep the rest of the logic (building result
with SavedPlanResponse.model_validate) the same; for long-term scalability
consider normalizing the training_dates array into a child table (e.g., a
PlanTrainingDate model) and query that table with a JOIN or EXISTS instead of
scanning SavedPlan in Python.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d9c81c9-5594-46b7-9ccb-a1954a38659b
📒 Files selected for processing (4)
src/api/v1/endpoints/plans.pysrc/db/models.pysrc/main.pysrc/models/plan_schema.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/models/plan_schema.py (1)
28-28: Consider mirroring request constraints in response schema.Optional: set
training_datesasField(..., min_length=1)inSavedPlanResponsetoo, so the API contract documents the non-empty invariant consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/plan_schema.py` at line 28, SavedPlanResponse currently defines training_dates without the non-empty constraint; mirror the request-side invariant by updating SavedPlanResponse.training_dates to use Field(..., min_length=1) (keeping the list[date] type) so the response schema documents the non-empty requirement; ensure you import Field from pydantic if not already and update the SavedPlanResponse class accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/models/plan_schema.py`:
- Line 28: SavedPlanResponse currently defines training_dates without the
non-empty constraint; mirror the request-side invariant by updating
SavedPlanResponse.training_dates to use Field(..., min_length=1) (keeping the
list[date] type) so the response schema documents the non-empty requirement;
ensure you import Field from pydantic if not already and update the
SavedPlanResponse class accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
어떤 변경사항인가요?
작업 상세 내용
SavedPlan모델:start_date(Date) →training_dates(JSON, list of date strings)SavePlanRequest/SavedPlanResponse스키마 변경 (training_dates: list[date])plans.py월 필터링 로직 변경 (training_dates중 해당 월 포함 여부)complete_plan_dayday_number 범위 검증을len(training_dates)기준으로saved_plans테이블 자동 마이그레이션 (start_date 컬럼 감지 → drop 후 재생성)체크리스트
관련 이슈
리뷰 포인트
training_dates는 JSON 컬럼에["2026-04-10", "2026-04-12"]형태로 저장됩니다.total_days는len(training_dates)로 자동 계산되어 저장됩니다.start_date컬럼 존재 여부로 판단하며, 마이그레이션 완료 후에는 no-op입니다.참고사항 및 스크린샷(선택)
프론트엔드(별도 레포) 변경사항:
training_dates: [date]로 변환training_dates직접 사용,addDays유틸 제거Summary by CodeRabbit
New Features
Bug Fixes
Chores