[WIKI-852] chore: update page version save logic #8440
Conversation
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRenames the page-version background task to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant View as PageView (partial_update)
participant Serializer
participant BG as track_page_version task
participant DB as PageVersion table
Client->>View: PATCH page (partial_update)
View->>View: store old_description_html
View->>Serializer: validate & serializer.save()
Serializer-->>View: saved page instance (page_id)
View->>BG: enqueue track_page_version(page_id, existing_instance, user_id)
Note right of BG: background worker executes later
BG->>DB: query latest PageVersion for page_id
alt latest exists && same user && within timeout
BG->>DB: update latest PageVersion in-place (description fields, sub_pages_data, updated_at)
else
BG->>DB: create new PageVersion record (with last_saved_at = now)
end
BG->>DB: prune to max 20 versions for page_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-10-17T08:23:54.961ZApplied to files:
📚 Learning: 2025-12-23T14:18:32.899ZApplied to files:
🧬 Code graph analysis (1)apps/api/plane/bgtasks/page_version_task.py (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
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 |
There was a problem hiding this comment.
Pull request overview
This pull request updates the page version saving logic to implement version consolidation within a time window, preventing excessive version creation during rapid edits.
Key Changes:
- Implements time-based version consolidation: updates existing versions if within 10 minutes and owned by same user, otherwise creates new versions
- Fixes order of operations in the view to ensure proper data capture before async tasks
- Adds
sub_pages_datafield tracking to page versions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
apps/api/plane/bgtasks/page_version_task.py |
Implements conditional logic to update existing page versions within a 10-minute window instead of always creating new ones, and adds sub_pages_data field |
apps/api/plane/app/views/page/base.py |
Fixes timing issue by capturing old description_html before save and moving serializer.save() before async task calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/plane/app/views/page/base.py (1)
556-568: Consider gatingpage_versioncall with description_html check.The
page_versiontask is invoked unconditionally (line 564), even whendescription_htmlis not being updated. While the task itself checks for changes (line 28 inpage_version_task.py), you could avoid unnecessary task enqueueing by gating it similarly topage_transaction:if request.data.get("description_html"): page_transaction.delay(...) page_version.delay(...)This would prevent enqueueing the task when no description changes are made.
apps/api/plane/bgtasks/page_version_task.py (1)
33-36: Simplify ownership comparison.The string conversion for comparing
owned_by_idwithuser_idis unnecessary. Since these are UUID fields, they can be compared directly:🔎 Suggested simplification
if ( page_version - and str(page_version.owned_by_id) == str(user_id) + and page_version.owned_by_id == user_id and (timezone.now() - page_version.last_saved_at).total_seconds() <= 600 ):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/app/views/page/base.pyapps/api/plane/bgtasks/page_version_task.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-17T08:23:54.961Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7966
File: apps/api/plane/bgtasks/page_transaction_task.py:108-127
Timestamp: 2025-10-17T08:23:54.961Z
Learning: In the page transaction system (apps/api/plane/bgtasks/page_transaction_task.py), entity names (entity_name) and entity identifiers (entity_identifier) for mentions and components remain constant once set and are not hardcoded values that change, so PageLog records don't need to handle updates to existing entity attributes.
Applied to files:
apps/api/plane/app/views/page/base.py
📚 Learning: 2025-12-23T14:18:32.899Z
Learnt from: dheeru0198
Repo: makeplane/plane PR: 8339
File: apps/api/plane/db/models/api.py:35-35
Timestamp: 2025-12-23T14:18:32.899Z
Learning: Django REST Framework rate limit strings are flexible: only the first character of the time unit matters. Acceptable formats include: "60/s", "60/sec", "60/second" (all equivalent), "60/m", "60/min", "60/minute" (all equivalent), "60/h", "60/hr", "60/hour" (all equivalent), and "60/d", "60/day" (all equivalent). Abbreviations like "min" are valid and do not need to be changed to "minute". Apply this guidance to any Python files in the project that configure DRF throttling rules.
Applied to files:
apps/api/plane/app/views/page/base.pyapps/api/plane/bgtasks/page_version_task.py
🧬 Code graph analysis (1)
apps/api/plane/app/views/page/base.py (3)
apps/api/plane/app/serializers/page.py (1)
PageBinaryUpdateSerializer(169-221)apps/api/plane/bgtasks/page_transaction_task.py (1)
page_transaction(81-138)apps/api/plane/bgtasks/page_version_task.py (1)
page_version(17-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/api/plane/app/views/page/base.py (2)
544-548: LGTM! Correctly captures state before update.The logic properly stores the original
description_htmlbefore the serializer modifies the page instance, then uses it to constructexisting_instancefor the version comparison in the background task. This ensures accurate version tracking.
553-553: LGTM! Explicit save ensures proper task coordination.Calling
serializer.save()explicitly before enqueueing background tasks ensures the page is persisted to the database, allowing the tasks to fetch the updated state reliably.apps/api/plane/bgtasks/page_version_task.py (2)
39-52: Verifylast_saved_atshould not be updated.When updating an existing
PageVersion, thelast_saved_atfield is not included in the update (lines 44-51). This means:
- The 10-minute consolidation window is fixed from when the version was first created, not extended with each update
- The timestamp won't reflect the actual last modification time
Example scenario:
- Version created at 12:00 PM
- Edit at 12:05 PM → updates same version (window: 5 min)
- Edit at 12:15 PM → creates new version (window: 15 min > 10 min)
Is this the intended behavior? For audit/display purposes, users might expect
last_saved_atto reflect the most recent save time.Alternative: Update last_saved_at on each save
If you want
last_saved_atto reflect the actual last modification time:page_version.description_html = page.description_html page_version.description_binary = page.description_binary page_version.description_json = page.description page_version.description_stripped = page.description_stripped page_version.sub_pages_data = sub_pages +page_version.last_saved_at = timezone.now() page_version.save( update_fields=[ "description_html", "description_binary", "description_json", "description_stripped", "sub_pages_data", + "last_saved_at", ] )Note: This would extend the consolidation window with each save, which may or may not be desired.
54-65: LGTM! New version creation logic is correct.The logic for creating a new
PageVersionwhen no suitable recent version exists is well-structured. It correctly usestimezone.now()forlast_saved_atand includes all necessary fields.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
NarayanBavisetti
force-pushed
the
chore-page-versioning-changes
branch
from
4fa92de to
2c22e55
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api/plane/bgtasks/page_version_task.py (1)
24-24: Clarify the purpose of the unusedsub_pagesvariable.The
sub_pagesvariable is initialized as an empty dictionary but never populated before being assigned tosub_pages_data(lines 43, 65). This issue has been flagged in multiple previous reviews.Is this intentionally a placeholder for future functionality, or should it be removed? If it's meant to store actual sub-page data, the extraction logic is missing.
🧹 Nitpick comments (1)
apps/api/plane/bgtasks/page_version_task.py (1)
30-36: Refactor for improved code clarity and maintainability.Several optional improvements would enhance code quality:
Line 30: The variable name
page_versioncould be more descriptive (e.g.,latest_page_version) to improve readability.Line 35: String conversion of both IDs is unnecessary since they should already be comparable types. Direct comparison would be more efficient:
page_version.owned_by_id == user_idLine 36: The hardcoded
600(10-minute window) should be extracted as a module-level constant likePAGE_VERSION_CONSOLIDATION_WINDOW_SECONDS = 600for better maintainability.🔎 Proposed refactor
At the top of the file, add:
+# Version consolidation window in seconds (10 minutes) +PAGE_VERSION_CONSOLIDATION_WINDOW_SECONDS = 600 + @shared_task def track_page_version(page_id, existing_instance, user_id):Then update the logic:
- page_version = PageVersion.objects.filter(page_id=page_id).order_by("-last_saved_at").first() + latest_page_version = PageVersion.objects.filter(page_id=page_id).order_by("-last_saved_at").first() # Get the latest page version if it exists and is owned by the user if ( - page_version - and str(page_version.owned_by_id) == str(user_id) - and (timezone.now() - page_version.last_saved_at).total_seconds() <= 600 + latest_page_version + and latest_page_version.owned_by_id == user_id + and (timezone.now() - latest_page_version.last_saved_at).total_seconds() <= PAGE_VERSION_CONSOLIDATION_WINDOW_SECONDS ): - page_version.description_html = page.description_html + latest_page_version.description_html = page.description_html # ... (update remaining fields similarly)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/app/views/page/base.pyapps/api/plane/bgtasks/page_version_task.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-17T08:23:54.961Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7966
File: apps/api/plane/bgtasks/page_transaction_task.py:108-127
Timestamp: 2025-10-17T08:23:54.961Z
Learning: In the page transaction system (apps/api/plane/bgtasks/page_transaction_task.py), entity names (entity_name) and entity identifiers (entity_identifier) for mentions and components remain constant once set and are not hardcoded values that change, so PageLog records don't need to handle updates to existing entity attributes.
Applied to files:
apps/api/plane/bgtasks/page_version_task.pyapps/api/plane/app/views/page/base.py
📚 Learning: 2025-12-23T14:18:32.899Z
Learnt from: dheeru0198
Repo: makeplane/plane PR: 8339
File: apps/api/plane/db/models/api.py:35-35
Timestamp: 2025-12-23T14:18:32.899Z
Learning: Django REST Framework rate limit strings are flexible: only the first character of the time unit matters. Acceptable formats include: "60/s", "60/sec", "60/second" (all equivalent), "60/m", "60/min", "60/minute" (all equivalent), "60/h", "60/hr", "60/hour" (all equivalent), and "60/d", "60/day" (all equivalent). Abbreviations like "min" are valid and do not need to be changed to "minute". Apply this guidance to any Python files in the project that configure DRF throttling rules.
Applied to files:
apps/api/plane/bgtasks/page_version_task.pyapps/api/plane/app/views/page/base.py
🧬 Code graph analysis (2)
apps/api/plane/bgtasks/page_version_task.py (3)
apps/api/plane/db/models/page.py (2)
Page(19-73)PageVersion(154-178)apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)apps/api/plane/app/views/page/base.py (2)
create(125-148)create(472-479)
apps/api/plane/app/views/page/base.py (3)
apps/api/plane/bgtasks/page_version_task.py (1)
track_page_version(17-77)apps/api/plane/app/serializers/page.py (1)
PageBinaryUpdateSerializer(169-221)apps/api/plane/bgtasks/page_transaction_task.py (1)
page_transaction(81-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
apps/api/plane/bgtasks/page_version_task.py (3)
1-17: LGTM: Clean imports and appropriate function rename.The addition of timezone utilities and the function rename from
page_versiontotrack_page_versionimproves clarity and aligns with the updated versioning logic.
38-66: LGTM: Version update and creation logic is correct.The conditional logic properly implements the intended behavior:
- Updates existing version when owned by same user and within 10 minutes
- Creates new version otherwise
- Correctly includes
updated_atin update_fields (line 51)- Intentionally does NOT update
last_saved_aton updates to maintain fixed 10-minute intervals (as clarified by maintainer)
67-77: LGTM: Version pruning and error handling are appropriate.The logic correctly maintains a cap of 20 versions per page and handles exceptions gracefully with proper logging.
apps/api/plane/app/views/page/base.py (3)
49-49: LGTM: Import updated to match renamed function.The import correctly references the renamed
track_page_versionfunction from the background task module.
544-548: LGTM: Correct capture of old description before updates.Storing
old_description_htmlbefore callingserializer.save()ensures both the page transaction and version tracking tasks have access to the correct pre-update state. This prevents race conditions and ensures accurate change tracking.
553-568: LGTM: Proper ordering of operations and correct task invocation.The changes demonstrate good practices:
serializer.save()is called before enqueuing background tasks, ensuring data consistency- The stored
old_description_htmlis correctly passed topage_transactiontrack_page_versionis invoked with the correct parameters (page_id,existing_instance,user_id) matching its updated signature
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/bgtasks/page_version_task.py (1)
68-71: Potential race condition in pruning logic.Between
count()andfirst().delete(), another process could delete versions. Iffirst()returnsNone(theoretically possible under concurrent load), calling.delete()on it raisesAttributeError.🔎 Proposed fix with null check
# If page versions are greater than 20 delete the oldest one if PageVersion.objects.filter(page_id=page_id).count() > 20: # Delete the old page version - PageVersion.objects.filter(page_id=page_id).order_by("last_saved_at").first().delete() + oldest_version = PageVersion.objects.filter(page_id=page_id).order_by("last_saved_at").first() + if oldest_version: + oldest_version.delete()
♻️ Duplicate comments (2)
apps/api/plane/bgtasks/page_version_task.py (2)
25-25:sub_pagesplaceholder remains unpopulated.This was flagged in previous reviews. The variable is initialized as an empty dictionary but never populated with actual data before being assigned to
sub_pages_data.
36-36: String conversion for UUID comparison is unnecessary.This was flagged in a previous review. Both
page_version.owned_by_idanduser_idshould be comparable directly without string conversion, which would be more efficient and type-safe.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/bgtasks/page_version_task.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-17T08:23:54.961Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7966
File: apps/api/plane/bgtasks/page_transaction_task.py:108-127
Timestamp: 2025-10-17T08:23:54.961Z
Learning: In the page transaction system (apps/api/plane/bgtasks/page_transaction_task.py), entity names (entity_name) and entity identifiers (entity_identifier) for mentions and components remain constant once set and are not hardcoded values that change, so PageLog records don't need to handle updates to existing entity attributes.
Applied to files:
apps/api/plane/bgtasks/page_version_task.py
📚 Learning: 2025-12-23T14:18:32.899Z
Learnt from: dheeru0198
Repo: makeplane/plane PR: 8339
File: apps/api/plane/db/models/api.py:35-35
Timestamp: 2025-12-23T14:18:32.899Z
Learning: Django REST Framework rate limit strings are flexible: only the first character of the time unit matters. Acceptable formats include: "60/s", "60/sec", "60/second" (all equivalent), "60/m", "60/min", "60/minute" (all equivalent), "60/h", "60/hr", "60/hour" (all equivalent), and "60/d", "60/day" (all equivalent). Abbreviations like "min" are valid and do not need to be changed to "minute". Apply this guidance to any Python files in the project that configure DRF throttling rules.
Applied to files:
apps/api/plane/bgtasks/page_version_task.py
🧬 Code graph analysis (1)
apps/api/plane/bgtasks/page_version_task.py (3)
apps/api/plane/db/models/page.py (2)
Page(19-73)PageVersion(154-178)apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)apps/api/plane/app/views/page/base.py (2)
create(125-148)create(472-479)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/api/plane/bgtasks/page_version_task.py (3)
15-15: Good extraction of the magic number into a named constant.This improves readability and maintainability by clearly documenting the 10-minute window for version consolidation.
40-54: Update logic looks correct.The inclusion of
updated_atinupdate_fieldsensures Django'sauto_nowbehavior works properly. The intentional omission oflast_saved_atmaintains fixed 10-minute intervals as designed.
73-78: Exception handling is appropriate for a background task.Silently returning on
Page.DoesNotExistis correct since the page may have been deleted between task enqueue and execution. Other exceptions are properly logged.
Description
This pull request updates the logic for saving page versions.
Type of Change
Note
Page versioning improvements
page_versiontask withtrack_page_version, adding a 10‑minute coalescing window to update the latestPageVersionfor the same user instead of creating duplicatesPagesDescriptionViewSet.partial_updatecapturesold_description_htmlbefore save for accuratepage_transactionand version comparisonsWritten by Cursor Bugbot for commit aece921. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.