Feature/reorder pages: Adds page ordering support as request of issue #4443#8733
Open
yevin215 wants to merge 3 commits intomakeplane:previewfrom
Open
Feature/reorder pages: Adds page ordering support as request of issue #4443#8733yevin215 wants to merge 3 commits intomakeplane:previewfrom
yevin215 wants to merge 3 commits intomakeplane:previewfrom
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughThis pull request introduces manual page reordering functionality by adding a sort_order field to the page model, implementing drag-and-drop UI components, and updating backend serialization and sorting logic. Additionally, it normalizes basePath handling in frontend config files and performs cleanup of deprecated settings. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Web App<br/>(React)
participant Store as Page Store<br/>(MobX)
participant API as Backend API
participant DB as Database
User->>Frontend: Drag page to new position
activate Frontend
Frontend->>Frontend: Detect drop, calculate edge<br/>(reorder-above/below)
Frontend->>Store: onPageDrop(sourceId, destId, edge)
deactivate Frontend
activate Store
Store->>Store: Compute new sort_order<br/>based on destination
Store->>API: PATCH /pages/{pageId}<br/>with sort_order update
Store->>Frontend: Optimistic UI update
deactivate Store
activate API
API->>DB: Update page.sort_order
DB-->>API: Confirmation
API-->>Store: Response with updated<br/>sort_order
deactivate API
alt Success
activate Frontend
Frontend->>User: Display reordered page<br/>in list
deactivate Frontend
else Failure
activate Store
Store->>Store: Rollback optimistic update
Store-->>Frontend: Error toast
deactivate Store
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/components/pages/list/block.tsx (1)
50-56:⚠️ Potential issue | 🔴 CriticalMove the new
useEffectabove the early return.The
if (!page) return nullcheck on line 50 precedes theuseEffecton line 54, violating React's Rules of Hooks. This causes the hook to be conditionally invoked depending on whetherpageexists, which can lead to hook mismatch errors and unpredictable behavior during store hydration.Fix
- // handle page check - if (!page) return null; - // derived values - const { name, logo_props, getRedirectionLink } = page; - useEffect(() => { const rowElement = parentRef.current; if (!rowElement || !isReorderEnabled) return; @@ }, [isReorderEnabled, isLastChild, onPageDrop, pageId]); + + // handle page check + if (!page) return null; + // derived values + const { name, logo_props, getRedirectionLink } = page;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/components/pages/list/block.tsx` around lines 50 - 56, The new useEffect is currently placed after the early return (if (!page) return null), causing conditional hook invocation; move the useEffect block that references parentRef and isReorderEnabled above the early return so hooks are always called; inside that effect, guard any usage of page-derived values (like name, logo_props, getRedirectionLink) with runtime checks (e.g., if (!page) return) to avoid dereferencing undefined while preserving the original logic in the effect.
🧹 Nitpick comments (4)
apps/space/vite.config.ts (1)
7-11: Consider extractingnormalizeBasePathto reduce duplication.This helper is duplicated across
apps/admin/vite.config.ts,apps/admin/react-router.config.ts,apps/space/vite.config.ts, andapps/space/react-router.config.ts. While acceptable for config files (which have build-time import constraints), extracting to a shared utility could improve maintainability if the logic ever needs adjustment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/space/vite.config.ts` around lines 7 - 11, The normalizeBasePath helper is duplicated across normalizeBasePath usages in apps/admin/vite.config.ts, apps/admin/react-router.config.ts, apps/space/vite.config.ts, and apps/space/react-router.config.ts; extract this function into a shared utility module (e.g., a new utils file inside a shared or config-utils package), export normalizeBasePath, and update the callers (those config files referencing normalizeBasePath) to import the shared function instead of defining it inline so future changes only need to be made in one place.apps/api/plane/app/views/page/base.py (1)
105-105: Consider adding an index onsort_orderfor performance at scale.The migration (context snippet 2) shows that
sort_orderwas added without a database index. Ordering by-sort_orderon every page list query could become slow as the number of pages grows. Consider adding an index in a follow-up migration if this endpoint is expected to handle large datasets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/app/views/page/base.py` at line 105, The query ordering uses "-sort_order" which lacks a DB index (see the Page model field sort_order and the migration that added it), causing potential slowdowns; add a follow-up migration to create an index on the sort_order column (and consider a composite index with is_favorite if queries commonly order by both) so the .order_by("-is_favorite", "-sort_order", "-created_at") call uses an indexed sort and improves performance at scale.apps/api/plane/tests/contract/app/test_pages_app.py (1)
29-60: Add a favorite-first assertion to this contract.All three pages here are non-favorites, so this only verifies descending
sort_order. The queryset now prepends-is_favorite, so please add oneUserFavoritefor the low-ranked page and assert it comes first despite its smallersort_order.Also applies to: 72-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/contract/app/test_pages_app.py` around lines 29 - 60, In project_pages (the fixture that creates Page and ProjectPage instances) create a UserFavorite for the low-ranked page (page_low) for the test user so that the queryset's new `-is_favorite` ordering can be exercised; then update the contract assertions that currently only check descending `sort_order` (both the block around project_pages and the similar block at lines 72-83) to assert that the favorited page_low appears first despite its lower sort_order, using the UserFavorite and the existing Page / ProjectPage objects to locate the expected first item.apps/web/core/components/pages/list/root.tsx (1)
58-63: Don't swallow the reorder error.Please capture the thrown error and route it through the existing logger/telemetry before showing the toast; otherwise reorder failures are opaque when this starts failing in production.
As per coding guidelines, "Use try-catch with proper error types and log errors appropriately for error handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/components/pages/list/root.tsx` around lines 58 - 63, The catch block in the page-reorder handler currently swallows errors; change the catch to capture the thrown error (e.g., catch (err)) and send it to the project's logging/telemetry before calling setToast (for example call logger.error(err) or telemetry.captureException(err)), and include a minimal error detail in the toast if appropriate; update references in this file to use the caught error variable when logging so failures are visible (affecting the catch in the reorder logic that calls setToast, TOAST_TYPE.ERROR and t()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/app/views/page/base.py`:
- Around line 103-105: The first .order_by(self.request.GET.get("order_by",
"-created_at")) is redundant because it gets overridden by the subsequent
.order_by("-is_favorite", "-sort_order", "-created_at"); remove the earlier
.order_by(...) call (the one using self.request.GET.get) from the queryset
building that also calls .prefetch_related("labels") and
.order_by("-is_favorite", "-sort_order", "-created_at"), or if user-controlled
ordering is required, refactor the logic so the user value is applied
after/combined with the fixed ordering instead of before it.
In `@apps/api/plane/celery.py`:
- Line 11: The codebase mixes import paths for python-json-logger; celery.py
uses "from pythonjsonlogger.json import JsonFormatter" while the settings
modules still import from "pythonjsonlogger.jsonlogger". Update the import
statements in the settings modules (where JsonFormatter is imported) to use
"pythonjsonlogger.json" to match celery.py (i.e., replace any
"pythonjsonlogger.jsonlogger" imports with "pythonjsonlogger.json" and keep the
same symbol names such as JsonFormatter).
In `@apps/web/core/components/pages/list/block.tsx`:
- Around line 115-118: The drag handle (rendered when isReorderEnabled)
currently uses a non-focusable <span> (dragHandleRef) and only supports pointer
drag via the DragHandle icon; add keyboard accessibility by replacing or
augmenting that span with a focusable control (e.g., a button or div with
tabIndex=0 and role="button") that uses the same dragHandleRef and exposes
keyboard handlers for reordering (ArrowUp/ArrowDown, Enter/Space to lift/drop)
or provide separate move-up/move-down buttons that call the same reordering
logic; ensure the control has an accessible label (aria-label or visually hidden
text) and preserves existing pointer drag behavior and styling in block.tsx.
In `@apps/web/core/store/pages/project-page.store.ts`:
- Around line 351-363: The current calculation uses orderedPageIds (which still
includes pageId) to compute destinationIndex/prevPageId and resultSequence,
causing no-op drops to produce new midpoints; update the logic in the block that
uses destinationSequence, orderedPageIds, destinationPageId, edge
("reorder-above"), getPageById and resultSequence to first build a newOrdered =
orderedPageIds.filter(id => id !== pageId), compute destinationIndex and
prevPageId from newOrdered (so the source is treated as removed), derive the
insertion slot from newOrdered, and if the computed slot equals the existing
slot (i.e., no change) return early without issuing the PATCH; otherwise proceed
to compute resultSequence as before using values from newOrdered/getPageById.
---
Outside diff comments:
In `@apps/web/core/components/pages/list/block.tsx`:
- Around line 50-56: The new useEffect is currently placed after the early
return (if (!page) return null), causing conditional hook invocation; move the
useEffect block that references parentRef and isReorderEnabled above the early
return so hooks are always called; inside that effect, guard any usage of
page-derived values (like name, logo_props, getRedirectionLink) with runtime
checks (e.g., if (!page) return) to avoid dereferencing undefined while
preserving the original logic in the effect.
---
Nitpick comments:
In `@apps/api/plane/app/views/page/base.py`:
- Line 105: The query ordering uses "-sort_order" which lacks a DB index (see
the Page model field sort_order and the migration that added it), causing
potential slowdowns; add a follow-up migration to create an index on the
sort_order column (and consider a composite index with is_favorite if queries
commonly order by both) so the .order_by("-is_favorite", "-sort_order",
"-created_at") call uses an indexed sort and improves performance at scale.
In `@apps/api/plane/tests/contract/app/test_pages_app.py`:
- Around line 29-60: In project_pages (the fixture that creates Page and
ProjectPage instances) create a UserFavorite for the low-ranked page (page_low)
for the test user so that the queryset's new `-is_favorite` ordering can be
exercised; then update the contract assertions that currently only check
descending `sort_order` (both the block around project_pages and the similar
block at lines 72-83) to assert that the favorited page_low appears first
despite its lower sort_order, using the UserFavorite and the existing Page /
ProjectPage objects to locate the expected first item.
In `@apps/space/vite.config.ts`:
- Around line 7-11: The normalizeBasePath helper is duplicated across
normalizeBasePath usages in apps/admin/vite.config.ts,
apps/admin/react-router.config.ts, apps/space/vite.config.ts, and
apps/space/react-router.config.ts; extract this function into a shared utility
module (e.g., a new utils file inside a shared or config-utils package), export
normalizeBasePath, and update the callers (those config files referencing
normalizeBasePath) to import the shared function instead of defining it inline
so future changes only need to be made in one place.
In `@apps/web/core/components/pages/list/root.tsx`:
- Around line 58-63: The catch block in the page-reorder handler currently
swallows errors; change the catch to capture the thrown error (e.g., catch
(err)) and send it to the project's logging/telemetry before calling setToast
(for example call logger.error(err) or telemetry.captureException(err)), and
include a minimal error detail in the toast if appropriate; update references in
this file to use the caught error variable when logging so failures are visible
(affecting the catch in the reorder logic that calls setToast, TOAST_TYPE.ERROR
and t()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c4701d9-942a-49b6-b0c9-8b2bc2aae50e
📒 Files selected for processing (17)
apps/admin/react-router.config.tsapps/admin/vite.config.tsapps/api/plane/app/serializers/page.pyapps/api/plane/app/views/page/base.pyapps/api/plane/celery.pyapps/api/plane/settings/common.pyapps/api/plane/settings/test.pyapps/api/plane/tests/contract/app/test_pages_app.pyapps/space/react-router.config.tsapps/space/vite.config.tsapps/web/core/components/pages/list/block.tsxapps/web/core/components/pages/list/order-by.tsxapps/web/core/components/pages/list/root.tsxapps/web/core/store/pages/base-page.tsapps/web/core/store/pages/project-page.store.tspackages/types/src/page/core.tspackages/utils/src/page.ts
💤 Files with no reviewable changes (1)
- apps/api/plane/settings/common.py
Comment on lines
103
to
+105
| .order_by(self.request.GET.get("order_by", "-created_at")) | ||
| .prefetch_related("labels") | ||
| .order_by("-is_favorite", "-created_at") | ||
| .order_by("-is_favorite", "-sort_order", "-created_at") |
Contributor
There was a problem hiding this comment.
Redundant order_by call on line 103.
Line 103 applies .order_by(self.request.GET.get("order_by", "-created_at")), but line 105 immediately overrides it with .order_by("-is_favorite", "-sort_order", "-created_at"). The first order_by has no effect.
If the user-supplied order_by parameter should be ignored in favor of the fixed ordering, consider removing line 103 to avoid confusion. If the intent was to allow user-controlled ordering, the logic needs restructuring.
Proposed fix to remove redundant order_by
.prefetch_related("projects")
.select_related("workspace")
.select_related("owned_by")
.annotate(is_favorite=Exists(subquery))
- .order_by(self.request.GET.get("order_by", "-created_at"))
.prefetch_related("labels")
.order_by("-is_favorite", "-sort_order", "-created_at")📝 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
| .order_by(self.request.GET.get("order_by", "-created_at")) | |
| .prefetch_related("labels") | |
| .order_by("-is_favorite", "-created_at") | |
| .order_by("-is_favorite", "-sort_order", "-created_at") | |
| .prefetch_related("labels") | |
| .order_by("-is_favorite", "-sort_order", "-created_at") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/app/views/page/base.py` around lines 103 - 105, The first
.order_by(self.request.GET.get("order_by", "-created_at")) is redundant because
it gets overridden by the subsequent .order_by("-is_favorite", "-sort_order",
"-created_at"); remove the earlier .order_by(...) call (the one using
self.request.GET.get) from the queryset building that also calls
.prefetch_related("labels") and .order_by("-is_favorite", "-sort_order",
"-created_at"), or if user-controlled ordering is required, refactor the logic
so the user value is applied after/combined with the fixed ordering instead of
before it.
| # Third party imports | ||
| from celery import Celery | ||
| from pythonjsonlogger.jsonlogger import JsonFormatter | ||
| from pythonjsonlogger.json import JsonFormatter |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Repo references =="
rg -n 'pythonjsonlogger\.(json|jsonlogger)\.JsonFormatter|from pythonjsonlogger\.(json|jsonlogger) import JsonFormatter' apps/api
echo
echo "== Pinned version =="
rg -n '^python-json-logger==' apps/api/requirements/base.txt
echo
echo "== Import check against pinned package =="
python -m pip install --quiet --disable-pip-version-check python-json-logger==3.3.0
python - <<'PY'
import importlib
for mod in ("pythonjsonlogger.json", "pythonjsonlogger.jsonlogger"):
try:
importlib.import_module(mod)
print(f"OK {mod}")
except Exception as exc:
print(f"FAIL {mod}: {type(exc).__name__}: {exc}")
PYRepository: makeplane/plane
Length of output: 513
Complete the python-json-logger import path migration for consistency.
This file imports via the new pythonjsonlogger.json path, but apps/api/plane/settings/production.py:37 and apps/api/plane/settings/local.py:49 still use pythonjsonlogger.jsonlogger. Both paths are available in the pinned version (v3.3.0), but the codebase should use the same path consistently. Update the settings to match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/celery.py` at line 11, The codebase mixes import paths for
python-json-logger; celery.py uses "from pythonjsonlogger.json import
JsonFormatter" while the settings modules still import from
"pythonjsonlogger.jsonlogger". Update the import statements in the settings
modules (where JsonFormatter is imported) to use "pythonjsonlogger.json" to
match celery.py (i.e., replace any "pythonjsonlogger.jsonlogger" imports with
"pythonjsonlogger.json" and keep the same symbol names such as JsonFormatter).
Comment on lines
+115
to
+118
| {isReorderEnabled && ( | ||
| <span ref={dragHandleRef} className="flex cursor-grab items-center text-placeholder"> | ||
| <DragHandle className="bg-transparent" /> | ||
| </span> |
Contributor
There was a problem hiding this comment.
Add a keyboard-accessible reorder path.
The new handle is a non-focusable <span> and the feature only exposes pointer drag-and-drop, so keyboard users currently have no way to reorder pages. Please provide a focusable control with keyboard semantics or alternate move-up/move-down actions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/core/components/pages/list/block.tsx` around lines 115 - 118, The
drag handle (rendered when isReorderEnabled) currently uses a non-focusable
<span> (dragHandleRef) and only supports pointer drag via the DragHandle icon;
add keyboard accessibility by replacing or augmenting that span with a focusable
control (e.g., a button or div with tabIndex=0 and role="button") that uses the
same dragHandleRef and exposes keyboard handlers for reordering
(ArrowUp/ArrowDown, Enter/Space to lift/drop) or provide separate
move-up/move-down buttons that call the same reordering logic; ensure the
control has an accessible label (aria-label or visually hidden text) and
preserves existing pointer drag behavior and styling in block.tsx.
Comment on lines
+351
to
+363
| const destinationSequence = destinationPage.sort_order; | ||
| if (typeof destinationSequence === "number") { | ||
| const destinationIndex = orderedPageIds.findIndex((id) => id === destinationPageId); | ||
| if (destinationIndex >= 0) { | ||
| if (edge === "reorder-above") { | ||
| const prevPageId = orderedPageIds[destinationIndex - 1]; | ||
| const prevSequence = prevPageId ? this.getPageById(prevPageId)?.sort_order : undefined; | ||
| if (typeof prevSequence === "number") resultSequence = (destinationSequence + prevSequence) / 2; | ||
| else resultSequence = destinationSequence + resultSequence; | ||
| } else { | ||
| resultSequence = destinationSequence - resultSequence; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Compute the insertion slot after removing the source page.
orderedPageIds still contains pageId, so a no-op drop like [A, B, …] → drop A “above” B still generates a fresh midpoint and sends an unnecessary PATCH. Please derive the insertion slot from the list with pageId removed and return early when that slot is unchanged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/core/store/pages/project-page.store.ts` around lines 351 - 363, The
current calculation uses orderedPageIds (which still includes pageId) to compute
destinationIndex/prevPageId and resultSequence, causing no-op drops to produce
new midpoints; update the logic in the block that uses destinationSequence,
orderedPageIds, destinationPageId, edge ("reorder-above"), getPageById and
resultSequence to first build a newOrdered = orderedPageIds.filter(id => id !==
pageId), compute destinationIndex and prevPageId from newOrdered (so the source
is treated as removed), derive the insertion slot from newOrdered, and if the
computed slot equals the existing slot (i.e., no change) return early without
issuing the PATCH; otherwise proceed to compute resultSequence as before using
values from newOrdered/getPageById.
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
2 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.
Description
Implement Manual Page Ordering
Back end
Front end
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Issue #4443
Summary by CodeRabbit
New Features
Chores