enable versioning and drafts for events collection#165
Conversation
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 4b8e143 in 1 minute and 10 seconds
More details
- Looked at
5789lines of code in5files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. src/collections/Events.ts:17
- Draft comment:
Versioning/drafts are enabled; ensure that the added “_status” field is consistently handled in UI and validations if needed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold70%
None
2. src/migrations/20250409_185348_versioning_drafts_events.ts:48
- Draft comment:
SQL in the migration uses DO $$ blocks with duplicate_object exception -- verify that this pattern is supported and consistent with other migrations. - Reason this comment was not posted:
Confidence changes required:50%<= threshold70%
None
3. src/migrations/index.ts:4
- Draft comment:
Migration file registration looks correct; ensure that order of migrations is intentional. - Reason this comment was not posted:
Confidence changes required:30%<= threshold70%
None
4. src/payload-types.ts:241
- Draft comment:
New _status field on Event interface is generated; verify that its optional status and union ('draft'|'published') meets UI and API expectations. - Reason this comment was not posted:
Confidence changes required:30%<= threshold70%
None
5. src/collections/Events.ts:17
- Draft comment:
Enables versioning with drafts and a maxPerDoc limit of 20. This configuration appears correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%
None
6. src/migrations/20250409_185348_versioning_drafts_events.ts:42
- Draft comment:
The migration drops NOT NULL constraints on start_date and end_date (and on slug, title, description, location) before adding _status. Verify that existing data meets these relaxed constraints. - Reason this comment was not posted:
Confidence changes required:50%<= threshold70%
None
7. src/migrations/20250409_185348_versioning_drafts_events.ts:48
- Draft comment:
Adds the _status column to the events table with default 'draft'. Confirm that the default meets your requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%
None
8. src/migrations/20250409_185348_versioning_drafts_events.ts:49
- Draft comment:
The DO blocks with EXCEPTION handling ensure FK constraints are only added if absent. This improves migration idempotence. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%
None
9. src/migrations/20250409_185348_versioning_drafts_events.ts:67
- Draft comment:
Conditional index creation via IF NOT EXISTS aids idempotence. The indexes on versioning tables and events _status look properly defined. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%
None
10. src/payload-types.ts:371
- Draft comment:
Typographical suggestion: In the Resource interface, the property for insurance details is named 'insuranceDetails' while its French counterpart is named 'insuranceDetailFr'. For consistency, consider renaming 'insuranceDetailFr' to 'insuranceDetailsFr'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_4xHcE8dbIJnIWdLu
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
🚀 Preview deployment is ready at https://pr-165.mindvista.ca. |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 484280d in 1 minute and 34 seconds
More details
- Looked at
27lines of code in1files - Skipped
1files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. src/migrations/20250409_185348_versioning_drafts_events.ts:82
- Draft comment:
Consider adding pagination or batching for the payload.update call. Updating all docs in one go may lead to performance issues for large datasets. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 70%
While batching large updates is generally good practice, this is a one-time migration script. The payload.update API may already handle batching internally. Without knowing the typical size of the events collection or the internal implementation of payload.update, this suggestion feels speculative. The current implementation is simple and clear.
I could be wrong about payload.update's internal implementation - it might try to load all documents into memory at once. The events collection could potentially be very large.
Even if the collection is large, this is a one-time migration that can be run during off-hours. The simple implementation is more maintainable and the performance impact would be temporary.
The comment should be removed as it's speculative without clear evidence of a problem, and complicating a one-time migration script may not be worth the tradeoff.
2. src/migrations/20250409_185348_versioning_drafts_events.ts:91
- Draft comment:
Confirm that the returned result structure (result.docs) is as expected. Ensure the migration handles cases where no documents are updated. - Reason this comment was not posted:
Confidence changes required:50%<= threshold70%
None
3. src/migrations/20250409_185348_versioning_drafts_events.ts:82
- Draft comment:
Consider filtering the update to only target documents not already marked as 'published', unless re-saving all is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%
None
4. src/migrations/20250409_185348_versioning_drafts_events.ts:84
- Draft comment:
Confirm that payload.update supports bulk updates and returns a 'docs' array, especially for large collections; consider batching if necessary. - Reason this comment was not posted:
Confidence changes required:66%<= threshold70%
None
5. src/migrations/20250409_185348_versioning_drafts_events.ts:91
- Draft comment:
Consider using a formal logging mechanism and verifying the existence of result.docs before accessing its length. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%
None
Workflow ID: wflow_U1JurrhTXxpkulcZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Code formatting check failed. The following files need formatting: Please run |
adds to #31
Important
Enable versioning and drafts for
eventscollection with migration and type updates.eventscollection inEvents.tswithmaxPerDocset to 20.eventsdocuments topublishedstatus to trigger versioning.20250409_185348_versioning_drafts_events.tsto create new tables_events_vand_events_v_version_date_ranges, and new typesenum_events_statusandenum__events_v_version_status.eventsandevents_date_rangestables to support new versioning and draft features.migrations/index.tsto include the new migration._statusfield toEventinterface inpayload-types.ts.This description was created by
for 484280d. It will automatically update as commits are pushed.