[Feature] - Adding Stickies reordering feature (Issue #8488)#8736
Open
Ivanq50 wants to merge 7 commits intomakeplane:previewfrom
Open
[Feature] - Adding Stickies reordering feature (Issue #8488)#8736Ivanq50 wants to merge 7 commits intomakeplane:previewfrom
Ivanq50 wants to merge 7 commits intomakeplane:previewfrom
Conversation
…-drop feature to the Stickies page to allow the user to manually reorder the stickies by priority of their choosing. Also changed from Masonry style stacking to horizontal line up.
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds manual reordering via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Browser
participant PageListBlock as UI_Component
participant Root as UI_Root
participant Store as ProjectPageStore
participant API as Backend_API
participant DB
User->>Browser: drag page item
Browser->>UI_Component: onDragStart (instruction data)
UI_Component->>Root: onPageDrop(sourceId,destId,edge)
Root->>Store: reorderPage(workspace, project, sourceId, destId, edge, orderedIds)
Store->>API: PATCH /api/projects/{id}/pages/{sourceId}/ (new sort_order)
API->>DB: update page.sort_order
DB-->>API: OK
API-->>Store: 200 updated page
Store-->>Root: success
Root-->>UI_Component: update UI, clear drag state
UI_Component-->>User: visual reorder applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds manual (drag-and-drop) reordering support for Stickies and Project Pages by introducing/using a sort_order field consistently across the web UI, shared utils/types, and the API, plus a few related config/test updates.
Changes:
- Add
sort_orderto page types/models usage and default page sorting to “manual order”, with DnD-driven reordering in the pages list UI. - Update Stickies layout to CSS Grid and add a drag-handle + drop indicators for Stickies reordering.
- Update API ordering/serialization to include
sort_order(and allowsort_orderupdates while a page is locked), with new contract tests and a few environment/config tweaks.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/src/page.ts | Adds sort_order as a supported sort key and switches to non-mutating reverse. |
| packages/types/src/page/core.ts | Extends page types/sort keys to include sort_order. |
| apps/web/core/store/sticky/sticky.store.ts | Adds guards + reordering logic updates; changes error rethrow behavior. |
| apps/web/core/store/pages/project-page.store.ts | Defaults page sorting to sort_order and adds reorderPage() store action. |
| apps/web/core/store/pages/base-page.ts | Adds sort_order to the mobx page model. |
| apps/web/core/components/stickies/layout/sticky-dnd-wrapper.tsx | Adds drag handle + drop indicators + wrapper styling for DnD. |
| apps/web/core/components/stickies/layout/stickies-list.tsx | Replaces Masonry layout with CSS Grid and adjusts layout helpers. |
| apps/web/core/components/pages/list/root.tsx | Wires pages list DnD drop handling and reordering UX/toasts. |
| apps/web/core/components/pages/list/order-by.tsx | Adds “Manual order” option to sort dropdown. |
| apps/web/core/components/pages/list/block.tsx | Implements per-row DnD (drag handle + drop targets) for pages list items. |
| apps/space/vite.config.ts | Replaces joinUrlPath usage with local base-path normalization. |
| apps/space/react-router.config.ts | Same base-path normalization change for React Router config. |
| apps/admin/vite.config.ts | Same base-path normalization change for admin app. |
| apps/admin/react-router.config.ts | Same base-path normalization change for admin router config. |
| apps/api/plane/tests/contract/app/test_pages_app.py | Adds contract tests for sort ordering and locked-page sort_order patch behavior. |
| apps/api/plane/tests/contract/api/test_sticky.py | Adds sticky ordering tests (currently problematic as written). |
| apps/api/plane/settings/test.py | Removes WhiteNoise middleware in tests to avoid static-dir warnings. |
| apps/api/plane/settings/common.py | Removes deprecated USE_L10N. |
| apps/api/plane/celery.py | Changes python-json-logger import path for JsonFormatter. |
| apps/api/plane/app/views/page/base.py | Orders pages by -sort_order and permits sort-only updates when locked. |
| apps/api/plane/app/serializers/page.py | Exposes sort_order in page serializer output. |
| apps/api/plane/api/views/sticky.py | Orders stickies by -sort_order (then -created_at). |
Comment on lines
244
to
246
| const destinationSequence = this.stickies[destinationId]?.sort_order || undefined; | ||
|
|
||
| if (destinationSequence) { |
There was a problem hiding this comment.
destinationSequence is derived using || undefined and later gated via truthy checks (if (destinationSequence)). This treats valid 0/negative sort_order values as “missing”. Since repeated reorders can drive sort_order down to 0/negative over time, this can break reordering (and the same issue applies to prevSequence checks later in this method). Use nullish coalescing and explicit numeric checks (e.g. typeof seq === "number").
| # Third party imports | ||
| from celery import Celery | ||
| from pythonjsonlogger.jsonlogger import JsonFormatter | ||
| from pythonjsonlogger.json import JsonFormatter |
There was a problem hiding this comment.
Import path for JsonFormatter was changed to pythonjsonlogger.json, but the repo still references pythonjsonlogger.jsonlogger.JsonFormatter in Django LOGGING settings. This inconsistency is likely to cause an ImportError at runtime depending on the installed python-json-logger layout; align the import with the configured formatter module (or update all references consistently).
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: t("toast.error"), | ||
| message: "Failed to reorder page. Please try again.", |
There was a problem hiding this comment.
The reorder failure toast message is hard-coded English while the component already uses useTranslation() for the title. To keep error messaging consistent and localizable, move the message string into i18n (e.g. t(...)) or reuse an existing translated key.
Suggested change
| message: "Failed to reorder page. Please try again.", | |
| message: t("toast.page_reorder_failed"), |
Comment on lines
+7
to
+11
| const normalizeBasePath = (value: string): string => { | ||
| const trimmed = value.trim(); | ||
| if (!trimmed || trimmed === "/") return "/"; | ||
| return `/${trimmed.replace(/^\/+|\/+$/g, "")}/`; | ||
| }; |
There was a problem hiding this comment.
normalizeBasePath is duplicated across multiple config files in this PR (admin + space, vite + react-router). Consider extracting a shared helper (or reusing the existing joinUrlPath util if it meets the requirements) to avoid future drift between apps/configs.
| handleLayout={handleLayout} | ||
| /> | ||
| {/* {!isInLastRow && <DropIndicator isVisible={instruction === "reorder-below"} />} */} | ||
| {!isLastChild && <DropIndicator isVisible={instruction === "reorder-below"} />} |
There was a problem hiding this comment.
The bottom drop indicator is rendered only when !isLastChild, but reorder-below is blocked for non-last items in getData (and only allowed for the last item). This makes the “drop below” affordance never appear where it’s actually valid. Render the reorder-below indicator for the last item (or align the block logic + indicator condition).
Suggested change
| {!isLastChild && <DropIndicator isVisible={instruction === "reorder-below"} />} | |
| {isLastChild && <DropIndicator isVisible={instruction === "reorder-below"} />} |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/space/react-router.config.ts (1)
9-13:⚠️ Potential issue | 🟡 MinorRemove trailing slash from
basenamenormalization to avoid double slashes in path composition.
normalizeBasePathadds trailing slashes (e.g.,/spaces/), which combined with React Router's basename handling and the codebase's explicit trailing-slash enforcement on navigation paths viaensureTrailingSlash(), produces double slashes in URLs (e.g.,/spaces//issues/anchor/). While React Router's routing is trailing-slash agnostic and will match routes correctly, this pattern is not recommended and violates best practices. The trailing slash is appropriate for Vite'sbaseoption but should be omitted for React Router'sbasename.Update the normalization for the React Router config to remove the trailing slash:
Suggested change
- return `/${trimmed.replace(/^\/+|\/+$/g, "")}/`; + return `/${trimmed.replace(/^\/+|\/+$/g, "")}`;Keep the trailing slash in
apps/space/vite.config.tsas Vite'sbaseoption requires it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/space/react-router.config.ts` around lines 9 - 13, The basename currently uses normalizeBasePath(process.env.VITE_SPACE_BASE_PATH ?? "") which returns a path with a trailing slash; update the assignment for basePath/basename so the exported React Router basename has no trailing slash (e.g., call normalizeBasePath(...) then strip any trailing "/" before assigning to the basename property). Locate the const basePath and the default export's basename in react-router.config.ts and ensure normalizeBasePath output is trimmed of a trailing slash while leaving Vite's base in apps/space/vite.config.ts unchanged.apps/web/core/store/sticky/sticky.store.ts (2)
194-200:⚠️ Potential issue | 🟠 MajorSnapshot the sticky before the optimistic mutation.
stickyis the same observable object mutated in theset(...)loop, so the catch path reassigns already-modified state. If the PATCH fails, the store still shows unsaved edits.💡 Suggested fix
updateSticky = async (workspaceSlug: string, id: string, updates: Partial<TSticky>) => { const sticky = this.stickies[id]; if (!sticky) return; + const previousSticky = { ...sticky }; try { runInAction(() => { Object.keys(updates).forEach((key) => { const currentStickyKey = key as keyof TSticky; set(this.stickies[id], key, updates[currentStickyKey] || undefined); @@ } catch (error) { console.error("Error in updating sticky:", error); - this.stickies[id] = sticky; + runInAction(() => { + this.stickies[id] = previousSticky; + }); throw new Error("", { cause: error }); } };Also applies to: 205-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/store/sticky/sticky.store.ts` around lines 194 - 200, Before mutating the observable sticky in runInAction, capture a snapshot of the current sticky (e.g., const prevSticky = { ...sticky } or a deep clone) so the catch path can restore the exact previous state; then perform the optimistic updates using set(this.stickies[id], ...), and in the catch handler restore the snapshot inside a runInAction by assigning this.stickies[id] = prevSticky (repeat the same snapshot/restore for the other optimistic mutation block that also uses set on this.stickies[id]).
235-252:⚠️ Potential issue | 🟠 MajorTreat
0as a validsort_orderwhen computing the new position.A
reorder-belowcan legitimately producesort_order === 0, but the|| undefined/ truthy checks here treat that as “missing” and leaveresultSequenceat10000. The same fallback also fires ifdestinationIdis stale or not part ofworkspaceStickies, so the sticky can jump to the top instead of remaining unchanged.💡 Suggested fix
if (stickyId === destinationId) return; const sticky = this.stickies[stickyId]; if (!sticky) return; + const destinationSticky = this.stickies[destinationId]; + if (!destinationSticky) return; const previousSortOrder = sticky.sort_order; try { let resultSequence = 10000; const workspaceStickies = this.workspaceStickies[workspaceSlug] || []; const stickies = workspaceStickies.map((id) => this.stickies[id]); const sortedStickies = orderBy(stickies, "sort_order", "desc").map((s) => s.id); - const destinationSequence = this.stickies[destinationId]?.sort_order || undefined; + const destinationSequence = destinationSticky.sort_order; + const destinationIndex = sortedStickies.findIndex((id) => id === destinationId); + if (destinationIndex === -1 || destinationSequence === undefined) return; - if (destinationSequence) { - const destinationIndex = sortedStickies.findIndex((id) => id === destinationId); - - if (edge === "reorder-above") { - const prevSequence = this.stickies[sortedStickies[destinationIndex - 1]]?.sort_order || undefined; - if (prevSequence) { + if (edge === "reorder-above") { + const prevSequence = this.stickies[sortedStickies[destinationIndex - 1]]?.sort_order; + if (prevSequence !== undefined) { resultSequence = (destinationSequence + prevSequence) / 2; } else { resultSequence = destinationSequence + resultSequence; } - } else { - // reorder-below - resultSequence = destinationSequence - resultSequence; - } + } else { + // reorder-below + resultSequence = destinationSequence - resultSequence; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/store/sticky/sticky.store.ts` around lines 235 - 252, The new-position computation treats 0 as falsy due to `|| undefined` and truthy checks, causing valid sort_order === 0 to be ignored and leaving resultSequence at 10000; update the logic in the reorder routine (referencing stickyId, destinationId, resultSequence, destinationSequence, workspaceStickies, sortedStickies and the "reorder-above"/"reorder-below" branches) to explicitly check for presence using strict null/undefined checks (e.g., !== undefined && !== null or Number.isFinite) instead of truthiness, and also ensure you verify destinationId is actually in workspaceStickies/sortedStickies before treating destinationSequence as missing so that 0 is correctly used when computing the averaged/adjacent sort_order.apps/web/core/components/stickies/layout/sticky-dnd-wrapper.tsx (1)
76-80:⚠️ Potential issue | 🟠 MajorDon’t re-register the DnD handlers on every drag state flip.
isDraggingis only used for styling, but it sits in the effect dependency list.onDragStart/onDropupdate that state, so the current draggable/drop target pair gets torn down and recreated during the gesture.Proposed fix
- }, [handleDrop, isDragging, isLastChild, isStickiesPage, stickyId, workspaceSlug]); + }, [handleDrop, isLastChild, isStickiesPage, stickyId, workspaceSlug]);Also applies to: 136-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/components/stickies/layout/sticky-dnd-wrapper.tsx` around lines 76 - 80, The drag handlers are causing the DnD system to re-register on every flip of isDragging; make the handlers stable so toggling isDragging (used only for styling) doesn't tear down the draggable/drop target. Extract the onDragStart and onDrop definitions in the sticky-dnd-wrapper component and memoize them (e.g., wrap in useCallback with stable deps or store as refs) so they are not recreated when isDragging changes; also remove isDragging from any effect dependency lists that drive DnD registration (ensure only stable handler references are used). Apply the same change for the other occurrence around the second handler pair referenced at line ~136.
🧹 Nitpick comments (2)
apps/space/vite.config.ts (1)
7-11: Consider extractingnormalizeBasePathto a shared utility.This helper function is duplicated identically across four config files (
apps/space/vite.config.ts,apps/space/react-router.config.ts,apps/admin/vite.config.ts,apps/admin/react-router.config.ts). Extracting it to a shared location (e.g.,packages/utilsor a localconfig-utils.ts) would reduce duplication and ensure consistent behavior if the logic ever needs adjustment.The implementation itself is sound—correctly handling empty strings, root paths, and stripping redundant slashes.
🤖 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 helper function normalizeBasePath is duplicated across multiple config files; extract it to a shared utility module (e.g., create a new config-utils.ts in a shared package like packages/utils) and export normalizeBasePath from there, then replace the local implementations in files that currently define normalizeBasePath (the vite.config.ts and react-router.config.ts modules in both apps) with an import of the shared function; ensure the exported function signature and behavior remain identical and update any imports/exports so TypeScript resolves the new module.apps/api/plane/api/views/sticky.py (1)
68-68: Add database index onsort_orderfor improved query performance.The
Sticky.sort_orderfield is the primary sort criterion in list operations but lacks a database index. This will cause full table scans on every query that orders by this field, becoming a performance bottleneck as workspace data grows.Add
db_index=Trueto thesort_orderfield inapps/api/plane/db/models/sticky.pyand create a migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/sticky.py` at line 68, The Sticky model's sort_order field needs a DB index: update the Sticky class definition to add db_index=True to the sort_order field (e.g., change its Field(...) to include db_index=True), then run Django makemigrations and migrate to create and apply the migration so the index is created in the database.
🤖 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 duplicate ordering is overriding the query param:
remove one of the order_by calls so ordering behaves as intended — either delete
the first order_by(self.request.GET.get("order_by", \"-created_at\")) if you
always want to enforce .order_by("-is_favorite", "-sort_order", "-created_at"),
or delete the later .order_by("-is_favorite", "-sort_order", "-created_at") if
the request GET order_by should take precedence; update the queryset chain that
uses .prefetch_related("labels") and .order_by(...) accordingly so only a single
.order_by(...) remains.
In `@apps/api/plane/tests/contract/api/test_sticky.py`:
- Around line 17-24: The test test_api_returns_ordered_list expects ascending
order but the API (StickyViewSet.list) currently orders by -sort_order
(descending); update the test assertions to match the API behavior by asserting
that results[0]["name"] == "Last" and results[1]["name"] == "First" (or
alternatively change the ordering in StickyViewSet.list to order by "sort_order"
if you want ascending behavior), referencing the Sticky model instances created
in the test and the StickyViewSet.list ordering to keep test and implementation
consistent.
- Around line 13-15: The test assertion in
apps/api/plane/tests/contract/api/test_sticky.py is using the wrong expected
sort_order for the second Sticky; the Sticky.save() logic in
apps/api/plane/db/models/sticky.py increments subsequent stickies by 10000 (base
65535), so update the assertion for s2 (created in the test) to
assertEqual(s2.sort_order, 75535) to match the model's behavior.
In `@apps/web/core/components/pages/list/block.tsx`:
- Around line 113-119: The drag handle is currently a non-focusable span so
keyboard and screen-reader users can't access reorder; replace the span used in
prependTitleElement with a focusable control (e.g., a button or an element with
role="button"/tabIndex=0) that uses the existing dragHandleRef and keeps the
DragHandle visual, add an accessible label (aria-label or aria-roledescription)
and keyboard handlers onKeyDown to support activation and keyboard reorder
semantics (e.g., Space/Enter to pick up/place item and Arrow keys or Home/End to
move while picked up) and ensure any drag state updates existing reorder
handlers (the same handlers used by the mouse drag logic) so both dragHandleRef
and DragHandle continue to work for assistive tech and keyboard users when
isReorderEnabled is true.
In `@apps/web/core/components/pages/list/root.tsx`:
- Around line 36-41: The drag-to-reorder toggle currently only checks that
filters.sortKey === "sort_order" but not the sort direction, so when the list is
sorted ascending a drag will compute wrong values in reorderPage; update the
canReorderPages expression to also require a descending sort direction (e.g.
ensure filters.sortBy !== "asc" or filters.sortDirection === "desc" depending on
the app's filter field) so reordering is enabled only when sort_order is present
and the list is descending, and keep the existing checks (pageType, searchQuery,
calculateTotalFilters, canCurrentUserCreatePage) intact.
In `@apps/web/core/components/stickies/layout/sticky-dnd-wrapper.tsx`:
- Around line 109-121: The drop-target logic mistakenly blocks "reorder-below"
for non-last children, so flip the condition where "reorder-below" is pushed
into blockedStates: push "reorder-below" only when isLastChild is true (or
equivalently, add it when isLastChild instead of when !isLastChild) so that
"reorder-below" is allowed for non-last items; make the same change for the
other occurrence around the attachInstruction call in the 149-168 block, keeping
the rest of the attachInstruction args (currentLevel, indentPerLevel, mode)
unchanged.
In `@apps/web/core/store/pages/project-page.store.ts`:
- Around line 353-362: The "reorder-below" branch currently subtracts a fixed
amount and can skip or tie the next sibling; update the else branch that handles
non-"reorder-above" to compute resultSequence between destinationSequence and
the next sibling's sort_order: use orderedPageIds[destinationIndex + 1] to get
nextPageId, call this.getPageById(nextPageId)?.sort_order to get nextSequence,
and if nextSequence is a number set resultSequence = (destinationSequence +
nextSequence) / 2, otherwise fall back to destinationSequence - 10000 (or the
existing fallback). This ensures resultSequence lands between the destination
and its next sibling and avoids jumping past or tying neighbors.
---
Outside diff comments:
In `@apps/space/react-router.config.ts`:
- Around line 9-13: The basename currently uses
normalizeBasePath(process.env.VITE_SPACE_BASE_PATH ?? "") which returns a path
with a trailing slash; update the assignment for basePath/basename so the
exported React Router basename has no trailing slash (e.g., call
normalizeBasePath(...) then strip any trailing "/" before assigning to the
basename property). Locate the const basePath and the default export's basename
in react-router.config.ts and ensure normalizeBasePath output is trimmed of a
trailing slash while leaving Vite's base in apps/space/vite.config.ts unchanged.
In `@apps/web/core/components/stickies/layout/sticky-dnd-wrapper.tsx`:
- Around line 76-80: The drag handlers are causing the DnD system to re-register
on every flip of isDragging; make the handlers stable so toggling isDragging
(used only for styling) doesn't tear down the draggable/drop target. Extract the
onDragStart and onDrop definitions in the sticky-dnd-wrapper component and
memoize them (e.g., wrap in useCallback with stable deps or store as refs) so
they are not recreated when isDragging changes; also remove isDragging from any
effect dependency lists that drive DnD registration (ensure only stable handler
references are used). Apply the same change for the other occurrence around the
second handler pair referenced at line ~136.
In `@apps/web/core/store/sticky/sticky.store.ts`:
- Around line 194-200: Before mutating the observable sticky in runInAction,
capture a snapshot of the current sticky (e.g., const prevSticky = { ...sticky }
or a deep clone) so the catch path can restore the exact previous state; then
perform the optimistic updates using set(this.stickies[id], ...), and in the
catch handler restore the snapshot inside a runInAction by assigning
this.stickies[id] = prevSticky (repeat the same snapshot/restore for the other
optimistic mutation block that also uses set on this.stickies[id]).
- Around line 235-252: The new-position computation treats 0 as falsy due to `||
undefined` and truthy checks, causing valid sort_order === 0 to be ignored and
leaving resultSequence at 10000; update the logic in the reorder routine
(referencing stickyId, destinationId, resultSequence, destinationSequence,
workspaceStickies, sortedStickies and the "reorder-above"/"reorder-below"
branches) to explicitly check for presence using strict null/undefined checks
(e.g., !== undefined && !== null or Number.isFinite) instead of truthiness, and
also ensure you verify destinationId is actually in
workspaceStickies/sortedStickies before treating destinationSequence as missing
so that 0 is correctly used when computing the averaged/adjacent sort_order.
---
Nitpick comments:
In `@apps/api/plane/api/views/sticky.py`:
- Line 68: The Sticky model's sort_order field needs a DB index: update the
Sticky class definition to add db_index=True to the sort_order field (e.g.,
change its Field(...) to include db_index=True), then run Django makemigrations
and migrate to create and apply the migration so the index is created in the
database.
In `@apps/space/vite.config.ts`:
- Around line 7-11: The helper function normalizeBasePath is duplicated across
multiple config files; extract it to a shared utility module (e.g., create a new
config-utils.ts in a shared package like packages/utils) and export
normalizeBasePath from there, then replace the local implementations in files
that currently define normalizeBasePath (the vite.config.ts and
react-router.config.ts modules in both apps) with an import of the shared
function; ensure the exported function signature and behavior remain identical
and update any imports/exports so TypeScript resolves the new module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0142a8ef-1e51-4df2-88d8-cf54779a50d4
📒 Files selected for processing (22)
apps/admin/react-router.config.tsapps/admin/vite.config.tsapps/api/plane/api/views/sticky.pyapps/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/api/test_sticky.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/components/stickies/layout/stickies-list.tsxapps/web/core/components/stickies/layout/sticky-dnd-wrapper.tsxapps/web/core/store/pages/base-page.tsapps/web/core/store/pages/project-page.store.tsapps/web/core/store/sticky/sticky.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.
Duplicate order_by() renders query parameter ineffective.
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"). In Django, only the last order_by() call takes effect.
If the intention is to always sort by favorites, sort_order, and created_at, remove Line 103. If the query parameter should take precedence, remove Line 105.
🔧 Proposed fix (remove the unused order_by)
.prefetch_related("labels")
- .order_by(self.request.GET.get("order_by", "-created_at"))
.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 duplicate
ordering is overriding the query param: remove one of the order_by calls so
ordering behaves as intended — either delete the first
order_by(self.request.GET.get("order_by", \"-created_at\")) if you always want
to enforce .order_by("-is_favorite", "-sort_order", "-created_at"), or delete
the later .order_by("-is_favorite", "-sort_order", "-created_at") if the request
GET order_by should take precedence; update the queryset chain that uses
.prefetch_related("labels") and .order_by(...) accordingly so only a single
.order_by(...) remains.
Comment on lines
+113
to
+119
| prependTitleElement={ | ||
| <> | ||
| {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.
The new drag handle is mouse-only.
This wraps the reorder affordance in a non-focusable span, so keyboard and screen-reader users have no way to trigger the new feature. Please expose a real control here and add a keyboard reorder path.
🤖 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 113 - 119, The
drag handle is currently a non-focusable span so keyboard and screen-reader
users can't access reorder; replace the span used in prependTitleElement with a
focusable control (e.g., a button or an element with role="button"/tabIndex=0)
that uses the existing dragHandleRef and keeps the DragHandle visual, add an
accessible label (aria-label or aria-roledescription) and keyboard handlers
onKeyDown to support activation and keyboard reorder semantics (e.g.,
Space/Enter to pick up/place item and Arrow keys or Home/End to move while
picked up) and ensure any drag state updates existing reorder handlers (the same
handlers used by the mouse drag logic) so both dragHandleRef and DragHandle
continue to work for assistive tech and keyboard users when isReorderEnabled is
true.
Comment on lines
+36
to
+41
| const canReorderPages = | ||
| pageType !== "archived" && | ||
| filters.sortKey === "sort_order" && | ||
| filters.searchQuery.trim().length === 0 && | ||
| calculateTotalFilters(filters.filters ?? {}) === 0 && | ||
| canCurrentUserCreatePage; |
Contributor
There was a problem hiding this comment.
Disable drag reorder when the list is sorted ascending.
reorderPage computes new values assuming the visible list is ordered by descending sort_order. If a user switches filters.sortBy to "asc", the same drag gesture will persist a value that moves the page the wrong way.
Proposed fix
const canReorderPages =
pageType !== "archived" &&
filters.sortKey === "sort_order" &&
+ filters.sortBy === "desc" &&
filters.searchQuery.trim().length === 0 &&
calculateTotalFilters(filters.filters ?? {}) === 0 &&
canCurrentUserCreatePage;📝 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
| const canReorderPages = | |
| pageType !== "archived" && | |
| filters.sortKey === "sort_order" && | |
| filters.searchQuery.trim().length === 0 && | |
| calculateTotalFilters(filters.filters ?? {}) === 0 && | |
| canCurrentUserCreatePage; | |
| const canReorderPages = | |
| pageType !== "archived" && | |
| filters.sortKey === "sort_order" && | |
| filters.sortBy === "desc" && | |
| filters.searchQuery.trim().length === 0 && | |
| calculateTotalFilters(filters.filters ?? {}) === 0 && | |
| canCurrentUserCreatePage; |
🤖 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 36 - 41, The
drag-to-reorder toggle currently only checks that filters.sortKey ===
"sort_order" but not the sort direction, so when the list is sorted ascending a
drag will compute wrong values in reorderPage; update the canReorderPages
expression to also require a descending sort direction (e.g. ensure
filters.sortBy !== "asc" or filters.sortDirection === "desc" depending on the
app's filter field) so reordering is enabled only when sort_order is present and
the list is descending, and keep the existing checks (pageType, searchQuery,
calculateTotalFilters, canCurrentUserCreatePage) intact.
Comment on lines
109
to
121
| const blockedStates: InstructionType[] = ["make-child"]; | ||
| if (!isLastChild) { | ||
| blockedStates.push("reorder-below"); | ||
| } | ||
|
|
||
| return attachInstruction(initialData, { | ||
| input, | ||
| element, | ||
| element: targetElement, | ||
| currentLevel: 1, | ||
| indentPerLevel: 0, | ||
| mode: isLastChild ? "last-in-group" : "standard", | ||
| block: blockedStates, | ||
| }); |
Contributor
There was a problem hiding this comment.
The append-to-end drop target is wired backwards.
reorder-below is only allowed when isLastChild is true, but the bottom indicator is rendered only when !isLastChild. That leaves the last sticky without a visible “drop after” target.
Proposed fix
- {!isLastChild && <DropIndicator isVisible={instruction === "reorder-below"} />}
+ {isLastChild && <DropIndicator isVisible={instruction === "reorder-below"} />}Also applies to: 149-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/core/components/stickies/layout/sticky-dnd-wrapper.tsx` around lines
109 - 121, The drop-target logic mistakenly blocks "reorder-below" for non-last
children, so flip the condition where "reorder-below" is pushed into
blockedStates: push "reorder-below" only when isLastChild is true (or
equivalently, add it when isLastChild instead of when !isLastChild) so that
"reorder-below" is allowed for non-last items; make the same change for the
other occurrence around the attachInstruction call in the 149-168 block, keeping
the rest of the attachInstruction args (currentLevel, indentPerLevel, mode)
unchanged.
Comment on lines
+353
to
+362
| 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.
reorder-below skips the next sibling.
For a descending list, subtracting a fixed 10000 only works when the destination is already last. Dropping below a middle page can jump past one or more pages or tie the next page’s sort_order, so the next fetch can come back in the wrong order.
Proposed fix
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;
+ const nextPageId = orderedPageIds[destinationIndex + 1];
+ const nextSequence = nextPageId ? this.getPageById(nextPageId)?.sort_order : undefined;
+ if (typeof nextSequence === "number") resultSequence = (destinationSequence + nextSequence) / 2;
+ else resultSequence = destinationSequence - resultSequence;
}📝 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
| 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; | |
| } | |
| 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 { | |
| const nextPageId = orderedPageIds[destinationIndex + 1]; | |
| const nextSequence = nextPageId ? this.getPageById(nextPageId)?.sort_order : undefined; | |
| if (typeof nextSequence === "number") resultSequence = (destinationSequence + nextSequence) / 2; | |
| else resultSequence = destinationSequence - resultSequence; | |
| } |
🤖 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 353 - 362, The
"reorder-below" branch currently subtracts a fixed amount and can skip or tie
the next sibling; update the else branch that handles non-"reorder-above" to
compute resultSequence between destinationSequence and the next sibling's
sort_order: use orderedPageIds[destinationIndex + 1] to get nextPageId, call
this.getPageById(nextPageId)?.sort_order to get nextSequence, and if
nextSequence is a number set resultSequence = (destinationSequence +
nextSequence) / 2, otherwise fall back to destinationSequence - 10000 (or the
existing fallback). This ensures resultSequence lands between the destination
and its next sibling and avoids jumping past or tying neighbors.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
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
Implemented the requested feature to allow the user to customize the order of the stickies. This feature only works on the Stickies tab (not on the home page via widget). Added a drag-handle to the Stickies to make it clear for the user where to select and drag. Changed the format from Masonry to CSS Grid as this fit the Stickies better.
Type of Change
Screenshots and Media (if applicable)
Before
After
Test Scenarios
Testing includes unit tests such as:
References
https://github.com/makeplane/plane/issues/8488
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests