Feature/on edit functionality#54
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds FieldRuleBook history/versioning with effective dates, persists prior versions on updates, resolves effective rule versions per-slot-date during slot sync, updates TypeORM wiring/migrations/DTOs/services, refactors pricing helpers to accept a RuleBook-like interface, and updates related API docs and examples. ChangesRule-book history, effective-date versioning, and slot pricing flow
API documentation and Bruno request examples
sequenceDiagram
participant Admin as rgba(40,167,69,0.5) Admin
participant API as rgba(0,123,255,0.5) API Server
participant Service as rgba(255,193,7,0.5) FieldsService
participant Sync as rgba(153,102,255,0.5) SlotSync
participant DB as rgba(220,53,69,0.5) Database
Admin->>API: Update/Create FieldRuleBook (effectiveDate)
API->>Service: persist FieldRuleBook (transaction)
Service->>DB: upsert previous state into FieldRuleBookHistory
Service->>DB: update FieldRuleBook.effective_date
Service->>Sync: trigger syncFieldWindow / syncFieldDate
Sync->>DB: query FieldRuleBook + FieldRuleBookHistory (<= slotDate)
Sync->>Sync: resolveRuleBooksForDate -> select latest RuleBookVersion
Sync->>Service: pass effective rule versions to pricing
Service->>DB: persist generated slots/booking changes
Service-->>Admin: return slotsUpdated status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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🔥 ProblemsGit: Failed to clone repository. Please run the 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: 11
🧹 Nitpick comments (2)
src/data-source.ts (1)
76-78: ⚡ Quick winInconsistent module loading: prefer ES6 import.
FieldRuleBookHistoryis loaded withrequire()while all other entities use ES6importstatements. This inconsistency can cause confusion and potential module resolution issues. Unless there's a specific circular dependency preventing a top-level import, prefer using a standard import statement for consistency.♻️ Refactor to use ES6 import
Add the import at the top of the file:
import { FieldSlot } from "./fields/entities/field-slot.entity"; +import { FieldRuleBookHistory } from "./fields/entities/field-rule-book-history.entity"; import { MembershipPlan } from "./booking/entities/membership-plan.entity";Then update the entities array:
FieldRuleBook, - // history entity - require("./fields/entities/field-rule-book-history.entity") - .FieldRuleBookHistory, + FieldRuleBookHistory, FieldSlot,🤖 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/data-source.ts` around lines 76 - 78, Replace the dynamic CommonJS require for FieldRuleBookHistory with a top-level ES6 import to match the rest of the file: add an import for FieldRuleBookHistory at the top of the module, then update the entities array to reference the imported FieldRuleBookHistory symbol instead of require(...). Ensure there are no circular-import issues; if one exists, refactor the dependent pieces or use a named re-export to resolve it before switching to the import.src/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts (1)
18-73: ⚡ Quick winTighten the new history columns to
NOT NULLafter backfill to match the entity.
FieldRuleBookHistorydeclaresruleName,slotSelectionType,actionType, andvalueas non-nullable (nonullable: true), but this migration adds them as nullable and never tightens them after the backfill. Once theUPDATEat lines 58-73 has populated every existing row, the columns should be promoted toNOT NULLso the DB schema matches the entity contract; otherwise a future external insert could produce NULL rows that the runtime treats as defined strings.The same applies, optionally, to
field_rule_books.effective_dateonce it is backfilled — though that one is sometimes intentionally nullable.♻️ Proposed addition after the backfills
await queryRunner.query(` UPDATE field_rule_book_history history SET rule_name = COALESCE(history.rule_name, rule_book.rule_name), slot_selection_type = COALESCE(history.slot_selection_type, rule_book.slot_selection_type), action_type = COALESCE(history.action_type, rule_book.action_type), value = COALESCE(history.value, rule_book.value) FROM field_rule_books rule_book WHERE history.rule_book_id = rule_book.id AND ( history.rule_name IS NULL OR history.slot_selection_type IS NULL OR history.action_type IS NULL OR history.value IS NULL ) `); + + await queryRunner.query(` + ALTER TABLE field_rule_book_history + ALTER COLUMN rule_name SET NOT NULL, + ALTER COLUMN slot_selection_type SET NOT NULL, + ALTER COLUMN action_type SET NOT NULL, + ALTER COLUMN value SET NOT NULL + `); }And mirror with
DROP NOT NULLindown()before dropping the columns.🤖 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/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts` around lines 18 - 73, The new history columns added via historyColumns (rule_name, slot_selection_type, action_type, value) are left nullable; after the backfill UPDATE that copies values from field_rule_books into field_rule_book_history, add ALTER TABLE statements to SET NOT NULL on those four columns (and optionally SET NOT NULL on field_rule_books.effective_date if you choose) so the DB matches the FieldRuleBookHistory entity; likewise, in down() before dropping the columns reverse that by ALTER TABLE ... DROP NOT NULL for each affected column, then proceed to drop them.
🤖 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 `@ApiDocs/HelloFutsal/environments/Dev.bru`:
- Line 4: The Dev.bru environment file contains a committed bearer value under
the token key; remove the actual token string, revoke/rotate it immediately,
replace it in Dev.bru with a non-sensitive placeholder (e.g. token: <REDACTED>
or TOKEN_PLACEHOLDER), and ensure the real secret is moved to an untracked
secret store (local env vars, .env.local added to .gitignore, or a secrets
manager) before committing; commit the placeholder change and document how to
inject the real token for local/dev use.
In `@ApiDocs/HelloFutsal/Rule` Book/EditRuleBook/EditTimeRange.bru:
- Line 10: The API spec shows "auth: none" but the request includes an
"Authorization: Bearer YOUR_TOKEN" header, creating an inconsistency; either
change the auth setting to "auth: bearer" (and keep the Authorization header) or
remove the Authorization header if the endpoint truly needs no auth—update the
auth line in EditTimeRange.bru and align the Authorization: Bearer YOUR_TOKEN
header accordingly (also apply the same fix to the similar occurrence noted at
line 14).
In `@BULK_BOOKING_API.md`:
- Around line 176-177: Update the BULK_BOOKING_API.md to remove the
contradiction between the "Transaction safety" bullet and the later "processing
continues after individual failures" statement by choosing one clear behavior
and making all related text consistent: either state the API is atomic (all
bookings succeed or the entire request rolls back) and describe rollback, error
codes, idempotency keys, and that the response will contain no booked slots on
failure; or state the API is best-effort (partial success allowed), describe
which slots persist, how failures are reported, the HTTP status code policy, and
idempotency handling. Edit the "Transaction safety" bullet and the paragraph
referencing "processing continues after individual failures" so both reflect the
chosen semantics and add a brief example and explicit note about idempotency and
rollback behavior to avoid ambiguity.
- Around line 143-149: The doc is inconsistent: the "409 Conflict" example block
(the JSON with "statusCode": 409 and "message": \"User with this phone number
already exists\") conflicts with other text that says "existing users are
reused"; pick one contract (either return 409 or reuse and return success) and
make the doc consistent. If you choose reuse, remove or replace the "409
Conflict" block and update any occurrences of the phrase "User with this phone
number already exists" and the headings "409 Conflict" to reflect a successful
reuse response; if you choose to keep 409, change the sections that currently
state "existing users are reused" to describe the conflict behavior and possible
remediation. Update all instances of the JSON example and the text "existing
users are reused" so they match the selected behavior.
- Around line 9-11: The fenced code block containing the endpoint "POST
/bookings/bulk/time-range" is missing a language identifier; update that fenced
block to include the language tag (e.g., add "http" after the opening backticks)
so the block begins with ```http, which satisfies markdownlint MD040 and enables
proper syntax highlighting for the endpoint.
In `@README.md`:
- Around line 97-98: The README contains an undocumented and dangerous
production reset command ("RESET_PROD_CONFIRM=RESET_PROD npm run db:reset:prod")
that is unrelated to the PR goals; remove this line from the README unless the
inclusion was intentional, and if intentional replace it with a dedicated
"Danger Zone: Production Database Reset" section that clearly labels the
command, explains purpose/risks, lists required safeguards (backups, approvals),
and documents when and by whom "db:reset:prod" and the "RESET_PROD_CONFIRM"
environment flag should be used.
In `@src/fields/cron/field-slot-cron.service.ts`:
- Around line 78-80: The cron decorator for
generateTomorrowSlotsAndBookMemberships was mistakenly set to
CronExpression.EVERY_MINUTE causing repeated runs; change the decorator on the
method (in FieldSlotCronService) to use CronExpression.EVERY_DAY_AT_MIDNIGHT
with the same timeZone ("Asia/Kathmandu") so the job runs once daily at
midnight; keep the existing method name and log messages ("Midnight slot cron
started/finished") and verify no other cron decorators for this method remain.
In `@src/fields/cron/field-slot-sync.service.ts`:
- Around line 445-495: The code builds versions by pushing the current ruleBook
first then its histories, then sorts by effectiveDate and returns the last
entry, which causes same-day ties to pick the older history version; fix by
ensuring the current ruleBook is considered newer on tie — e.g., push history
entries from historiesByRuleBookId.get(ruleBook.id) into versions before pushing
the current ruleBook (or adjust the sort comparator to break ties in favor of
the current ruleBook by comparing a tiebreaker such as source === "current" or
createdAt), updating the block that constructs versions (the versions array, the
loop over historiesByRuleBookId, and the push of the current ruleBook) so that
when effectiveDate === effectiveFromDate the current ruleBook wins.
In `@src/fields/entities/field-rule-book-history.entity.ts`:
- Around line 28-38: The entity fields ruleName, slotSelectionType, actionType,
and value are declared non-nullable in FieldRuleBookHistory (ruleName,
slotSelectionType, actionType, value) but the migration
1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts creates them
nullable; either make the entity nullable types or, preferably, update the
migration to SET NOT NULL after the safe backfill: after copying values from the
parent rule book, execute ALTER TABLE ... ALTER COLUMN "<column>" SET NOT NULL
for each of rule_name, slot_selection_type, action_type and value so the DB
schema matches the non-nullable declarations in the entity.
In `@src/fields/fields.service.ts`:
- Around line 1030-1070: Replace the ad-hoc casting and UTC today logic by
reading effectiveDate from the typed DTO (createFieldRuleBookDto.effectiveDate)
and using FieldSlotGenerator.getCurrentDateString() for the default, assign to
appliedEffectiveDate, then validate temporal ordering: compute
previousEffectiveDate from existingRuleBook.effectiveDate or
existingRuleBook.createdAt as before and if appliedEffectiveDate <
previousEffectiveDate throw a BadRequestException; ensure this check runs before
writing history or saving (inside the transaction) so resolveRuleBooksForDate
sees consistent ordering.
- Around line 943-946: Remove the unnecessary cast and replace the UTC-based
default date with the shared helper: use createFieldRuleBookDto.effectiveDate
(no "as any") and fall back to FieldSlotGenerator.getCurrentDateString() instead
of new Date().toISOString().split("T")[0] when setting effectiveDate in the
create method (look for effectiveDate and createFieldRuleBookDto) and apply the
identical change in updateFieldRuleBook (lines referenced around
updateFieldRuleBook) so both methods use the same local-time date helper for
consistency.
---
Nitpick comments:
In `@src/data-source.ts`:
- Around line 76-78: Replace the dynamic CommonJS require for
FieldRuleBookHistory with a top-level ES6 import to match the rest of the file:
add an import for FieldRuleBookHistory at the top of the module, then update the
entities array to reference the imported FieldRuleBookHistory symbol instead of
require(...). Ensure there are no circular-import issues; if one exists,
refactor the dependent pieces or use a named re-export to resolve it before
switching to the import.
In `@src/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts`:
- Around line 18-73: The new history columns added via historyColumns
(rule_name, slot_selection_type, action_type, value) are left nullable; after
the backfill UPDATE that copies values from field_rule_books into
field_rule_book_history, add ALTER TABLE statements to SET NOT NULL on those
four columns (and optionally SET NOT NULL on field_rule_books.effective_date if
you choose) so the DB matches the FieldRuleBookHistory entity; likewise, in
down() before dropping the columns reverse that by ALTER TABLE ... DROP NOT NULL
for each affected column, then proceed to drop them.
🪄 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: 1a664f53-1865-479a-8dba-c7cc84cd4a63
📒 Files selected for processing (21)
ApiDocs/HelloFutsal/Booking/BulkBookSlots.bruApiDocs/HelloFutsal/Rule Book/EditRuleBook/EditTimeRange.bruApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookAllSlots.bruApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookSpecificSlots.bruApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookTimeRange.bruApiDocs/HelloFutsal/VerifyOTP/Admin OTP Verify.bruApiDocs/HelloFutsal/environments/Dev.bruBULK_BOOKING_API.mdREADME.mdsrc/app.module.tssrc/data-source.tssrc/fields/cron/field-slot-cron.service.tssrc/fields/cron/field-slot-generator.tssrc/fields/cron/field-slot-sync.service.tssrc/fields/dto/create-field-rule-book.dto.tssrc/fields/entities/field-rule-book-history.entity.tssrc/fields/entities/field-rule-book.entity.tssrc/fields/fields.module.tssrc/fields/fields.service.tssrc/migrations/1780000000004-CreateFieldRuleBookHistory.tssrc/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts
💤 Files with no reviewable changes (4)
- ApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookTimeRange.bru
- ApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookSpecificSlots.bru
- ApiDocs/HelloFutsal/Booking/BulkBookSlots.bru
- ApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookAllSlots.bru
| fieldId: 1bdfdb6b-41d8-4e39-a38c-398bdca0ba39 | ||
| token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIzMDIyMWFkOS0zZjgyLTQzZDgtYjQ1OS1kZGYyMGY2YzgzOTAiLCJlbWFpbCI6bnVsbCwibW9iaWxlTnVtYmVyIjoiOTg2Nzc1NDczOCIsInJvbGUiOiJhZG1pbiIsImlhdCI6MTc3ODMwMDMyOSwiZXhwIjoxNzc4Mzg2NzI5fQ.GHeUq7DIOmimuj6eBVFtFq6HiFXf9oSQP8RS0pW1sCU | ||
| fieldId: 80607ec4-9caf-4e38-a7d2-337e5c2343bb | ||
| token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxNzQ5MWQxMy1hMGEwLTRlNjctYTg1OC0yN2FlYWZmYTQxMTgiLCJlbWFpbCI6bnVsbCwibW9iaWxlTnVtYmVyIjoiOTg2Nzc1NDczOCIsInJvbGUiOiJhZG1pbiIsImlhdCI6MTc3ODQyNDYxOCwiZXhwIjoxNzc4NTExMDE4fQ.dhztQY8hLUCCXt_MRx4TcCp9O_7SBuw6tZqJONjVLis |
There was a problem hiding this comment.
Remove committed bearer token and rotate it immediately.
Line 4 contains a real credential in source control. This is a secret/PII exposure risk (and potentially privileged access). Revoke/rotate the token now, and keep only a placeholder in tracked env files while storing the real value in an untracked local secret source.
Suggested tracked-file change
- token: <hardcoded-jwt>
+ token: __SET_LOCALLY_DO_NOT_COMMIT__🤖 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 `@ApiDocs/HelloFutsal/environments/Dev.bru` at line 4, The Dev.bru environment
file contains a committed bearer value under the token key; remove the actual
token string, revoke/rotate it immediately, replace it in Dev.bru with a
non-sensitive placeholder (e.g. token: <REDACTED> or TOKEN_PLACEHOLDER), and
ensure the real secret is moved to an untracked secret store (local env vars,
.env.local added to .gitignore, or a secrets manager) before committing; commit
the placeholder change and document how to inject the real token for local/dev
use.
Comment on lines
+9
to
+11
| ``` | ||
| POST /bookings/bulk/time-range | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to fenced code block.
The endpoint code fence is missing a language tag (markdownlint MD040).
Suggested doc fix
-```
+```http
POST /bookings/bulk/time-range</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@BULK_BOOKING_API.md` around lines 9 - 11, The fenced code block containing
the endpoint "POST /bookings/bulk/time-range" is missing a language identifier;
update that fenced block to include the language tag (e.g., add "http" after the
opening backticks) so the block begins with ```http, which satisfies
markdownlint MD040 and enables proper syntax highlighting for the endpoint.
Comment on lines
+143
to
+149
| **409 Conflict** (User with phone number already exists) | ||
|
|
||
| ```json | ||
| { | ||
| "statusCode": 409, | ||
| "message": "User with this phone number already exists" | ||
| } |
There was a problem hiding this comment.
User existence behavior is documented inconsistently.
Line 143-149 documents 409 Conflict when phone number exists, but Line 157 and Line 225 state existing users are reused. Please pick one contract and keep error examples and flow description consistent.
Also applies to: 157-157, 225-225
🤖 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 `@BULK_BOOKING_API.md` around lines 143 - 149, The doc is inconsistent: the
"409 Conflict" example block (the JSON with "statusCode": 409 and "message":
\"User with this phone number already exists\") conflicts with other text that
says "existing users are reused"; pick one contract (either return 409 or reuse
and return success) and make the doc consistent. If you choose reuse, remove or
replace the "409 Conflict" block and update any occurrences of the phrase "User
with this phone number already exists" and the headings "409 Conflict" to
reflect a successful reuse response; if you choose to keep 409, change the
sections that currently state "existing users are reused" to describe the
conflict behavior and possible remediation. Update all instances of the JSON
example and the text "existing users are reused" so they match the selected
behavior.
Comment on lines
+176
to
+177
| ✅ **Transaction safety** - All bookings are atomic (all succeed or all fail) | ||
| ✅ **Detailed response** - Shows booked slots and failures with reasons |
There was a problem hiding this comment.
Resolve contradictory transaction semantics in docs.
Line 176 says atomic all-or-nothing behavior, but Line 227 says processing continues after individual failures. These are mutually exclusive and will mislead API clients about failure handling and idempotency expectations. Align this section to one clear behavior.
Suggested doc fix
-✅ **Transaction safety** - All bookings are atomic (all succeed or all fail)
+✅ **Concurrency safety** - Uses pessimistic locking and transactional writes per booking attempt
...
-- The API processes slots sequentially; if one booking fails, others continue
+- The API processes slots sequentially; failed slots are reported in `failedBookings` while successful slots remain bookedAlso applies to: 227-227
🤖 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 `@BULK_BOOKING_API.md` around lines 176 - 177, Update the BULK_BOOKING_API.md
to remove the contradiction between the "Transaction safety" bullet and the
later "processing continues after individual failures" statement by choosing one
clear behavior and making all related text consistent: either state the API is
atomic (all bookings succeed or the entire request rolls back) and describe
rollback, error codes, idempotency keys, and that the response will contain no
booked slots on failure; or state the API is best-effort (partial success
allowed), describe which slots persist, how failures are reported, the HTTP
status code policy, and idempotency handling. Edit the "Transaction safety"
bullet and the paragraph referencing "processing continues after individual
failures" so both reflect the chosen semantics and add a brief example and
explicit note about idempotency and rollback behavior to avoid ambiguity.
…nd bearer auth; enhance effective date handling in FieldsService and migrations
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ApiDocs/HelloFutsal/Rule` Book/EditRuleBook/EditTimeRange.bru:
- Line 24: Replace the hard-coded calendar date used in the request example for
the "effectiveDate" field (currently "2026-05-21") with a placeholder or example
format (e.g., "YYYY-MM-DD" or "{effectiveDate}") so consumers don't copy stale
values; update the EditTimeRange example where "effectiveDate" is defined to use
the placeholder and, if desired, add a brief note about the expected ISO date
format.
In `@src/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts`:
- Around line 58-80: The migration currently backfills field_rule_book_history
from the live field_rule_books (the UPDATE query and subsequent ALTER TABLE ...
SET NOT NULL), which overwrites historical rows; remove the UPDATE and do not
make existing history columns NOT NULL. Instead add the new columns as nullable
(leave rule_name, slot_selection_type, action_type, value nullable for legacy
rows) and enforce non-null only for new history records (e.g. via a DB trigger
or a partial constraint/index that applies to rows with created_at >= migration
time or by enforcing at the application/insert path). Update the migration code
in AddEffectiveDateAndVersionFieldsToRuleBooks.ts (the queryRunner.query calls
that run the UPDATE and ALTER TABLE) to drop the backfill UPDATE and the
immediate ALTER COLUMN ... SET NOT NULL, and add a follow-up plan to enforce NOT
NULL for future inserts-only using a trigger or partial constraint.
🪄 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: 2c0f1d01-255d-4dfd-bb9d-6db23ad44a60
📒 Files selected for processing (5)
ApiDocs/HelloFutsal/Rule Book/EditRuleBook/EditTimeRange.bruApiDocs/HelloFutsal/environments/Dev.brusrc/fields/cron/field-slot-sync.service.tssrc/fields/fields.service.tssrc/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ApiDocs/HelloFutsal/environments/Dev.bru
- src/fields/fields.service.ts
- src/fields/cron/field-slot-sync.service.ts
|
|
||
| body:json { | ||
| { | ||
| "effectiveDate": "2026-05-21", |
There was a problem hiding this comment.
Avoid hard-coded calendar dates in request examples.
Line 24 uses a fixed date ("2026-05-21"), which will become stale and can mislead API consumers when copied. Prefer a placeholder/example format.
Suggested doc tweak
- "effectiveDate": "2026-05-21",
+ "effectiveDate": "YYYY-MM-DD",📝 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
| "effectiveDate": "2026-05-21", | |
| "effectiveDate": "YYYY-MM-DD", |
🤖 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 `@ApiDocs/HelloFutsal/Rule` Book/EditRuleBook/EditTimeRange.bru at line 24,
Replace the hard-coded calendar date used in the request example for the
"effectiveDate" field (currently "2026-05-21") with a placeholder or example
format (e.g., "YYYY-MM-DD" or "{effectiveDate}") so consumers don't copy stale
values; update the EditTimeRange example where "effectiveDate" is defined to use
the placeholder and, if desired, add a brief note about the expected ISO date
format.
…rule_book_history
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts`:
- Around line 5-15: The migration's up()/down() are asymmetric: up() currently
skips creating columns if they already exist (ruleBooksTable.findColumnByName)
but later code always rewrites and down() will drop columns even if the
migration didn't create them; update up() to fail-fast if any target columns
(effective_date, version or whichever columns handled in this file) already
exist by throwing an Error after using queryRunner.getTable and
findColumnByName, and keep the existing addColumn/addColumns logic otherwise so
down() can safely drop what up() created; alternatively persist a creation
marker (e.g., a small migration-owned meta-row) so down() only drops when that
marker exists — implement either option inside the migration's up()/down()
methods and reference the same column names and queryRunner methods (getTable,
addColumn, dropColumn) to ensure symmetry.
🪄 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: 6c504b82-2b0c-43bd-845f-a3c4191adbc9
📒 Files selected for processing (1)
src/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts
Comment on lines
+5
to
+15
| const ruleBooksTable = await queryRunner.getTable("field_rule_books"); | ||
| if (!ruleBooksTable?.findColumnByName("effective_date")) { | ||
| await queryRunner.addColumn( | ||
| "field_rule_books", | ||
| new TableColumn({ | ||
| name: "effective_date", | ||
| type: "date", | ||
| isNullable: true, | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The guarded up() logic is not symmetric with the backfill and rollback.
Lines 5-50 treat pre-existing columns as valid, but Lines 52-56 still rewrite effective_date, and Lines 62-78 will drop any matching columns on rollback even if this migration did not create them. On a drifted or manually patched database, that can mutate or delete schema/data this migration does not own. Either fail fast on unexpected schema instead of using findColumnByName, or persist enough state to make down() revert only what up() actually added.
Also applies to: 17-50, 52-56, 62-78
🤖 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/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts`
around lines 5 - 15, The migration's up()/down() are asymmetric: up() currently
skips creating columns if they already exist (ruleBooksTable.findColumnByName)
but later code always rewrites and down() will drop columns even if the
migration didn't create them; update up() to fail-fast if any target columns
(effective_date, version or whichever columns handled in this file) already
exist by throwing an Error after using queryRunner.getTable and
findColumnByName, and keep the existing addColumn/addColumns logic otherwise so
down() can safely drop what up() created; alternatively persist a creation
marker (e.g., a small migration-owned meta-row) so down() only drops when that
marker exists — implement either option inside the migration's up()/down()
methods and reference the same column names and queryRunner methods (getTable,
addColumn, dropColumn) to ensure symmetry.
…e and history columns
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores