Feat/Lexorank implementation for roadmaps #1452
Open
Conversation
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
📝 WalkthroughWalkthroughThis PR replaces integer-based roadmap indexing with LexoRank-based lexicographic ranking. Changes include adding the lexorank dependency, updating database schema and migrations, refactoring the create and sort endpoints with LexoRank logic, updating frontend drag-drop handlers to call the new sort API with revised parameters, and updating type definitions across the stack. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend<br/>(TabularView)
participant Module as Module<br/>(roadmaps.ts)
participant API as API<br/>(sort endpoint)
participant Store as Store<br/>(dashboard/roadmaps)
participant DB as Database
rect rgb(200, 220, 240)
Note over Frontend,Store: Drag End Handler
User->>Frontend: Release dragged roadmap
Frontend->>Frontend: onDragEnd(oldIndex, newIndex)
Frontend->>Frontend: Compute dragged roadmap ID<br/>and adjacent prev/next IDs
alt Positions unchanged
Frontend->>Frontend: Return (no-op)
else Valid reorder
Frontend->>Module: sortRoadmap(id, prevId, nextId)
end
end
rect rgb(220, 240, 220)
Note over Module,DB: Sort via API
Module->>API: PATCH /api/v1/roadmaps/sort<br/>{ id, prevRoadmapId, nextRoadmapId }
API->>API: Validate schema & permissions
API->>DB: Fetch prev/next roadmaps
API->>API: calculateRankIndex(prev, next)<br/>using LexoRank
API->>DB: Update roadmap index (transaction)
API->>Module: 200 { index: "<LexoRank>" }
end
rect rgb(240, 220, 220)
Note over Frontend,Store: Update UI State
Module->>Store: updateRoadmapIndex(roadmapId, newIndex)
Store->>Store: Locate roadmap, update index
Store->>Frontend: Return updated index
Frontend->>Frontend: UI reflects new ordering
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
Lighthouse report
|
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/server/tests/utils/generators.ts (1)
44-69: Restore roadmap override values in the test generator.
generateRoadmapnow hard-codesindex,color, anddisplay, ignoring any values passed via theroadmapargument. The GET tests rely ongenerateRoadmap({ display: isPublic })to seed 50 public vs 50 private entries; with the new code this becomes random, making those tests (and any others needing deterministic fixtures) fail or flake. Please reinstate the previousroadmap?.display ?? …/roadmap?.color ?? …fallback (and similar forindexif callers supply one).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
packages/server/package.json(1 hunks)packages/server/src/database/migrations/20201224104136_add-roadmaps-table.ts(1 hunks)packages/server/src/ee/controllers/v1/roadmaps/create.ts(2 hunks)packages/server/src/ee/controllers/v1/roadmaps/sort.ts(4 hunks)packages/server/src/errorResponse.json(1 hunks)packages/server/tests/integration/v1/roadmaps.spec.ts(3 hunks)packages/server/tests/utils/generators.ts(2 hunks)packages/theme/package.json(1 hunks)packages/theme/src/ee/components/dashboard/roadmap/TabularView.vue(2 hunks)packages/theme/src/ee/modules/roadmaps.ts(2 hunks)packages/theme/src/ee/store/dashboard/roadmaps.ts(4 hunks)packages/types/src/roadmap.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/server/src/ee/controllers/v1/roadmaps/create.ts (1)
packages/theme/src/ee/modules/roadmaps.ts (1)
createRoadmap(22-37)
packages/server/tests/utils/generators.ts (1)
packages/server/src/helpers.ts (1)
sanitiseURL(190-190)
packages/theme/src/ee/store/dashboard/roadmaps.ts (3)
packages/types/src/roadmap.ts (1)
IRoadmapPrivate(14-18)packages/theme/src/ee/modules/roadmaps.ts (2)
updateRoadmap(49-64)sortRoadmap(69-90)packages/server/src/ee/controllers/v1/roadmaps/updateRoadmap.ts (1)
updateRoadmap(58-127)
packages/server/src/ee/controllers/v1/roadmaps/sort.ts (1)
packages/types/src/roadmap.ts (1)
ISortRoadmapRequestBody(67-71)
packages/server/tests/integration/v1/roadmaps.spec.ts (3)
packages/server/tests/utils/seed/user.ts (1)
createUser(27-73)packages/server/tests/utils/createRoleWithPermissions.ts (1)
createRoleWithPermissions(10-58)packages/types/src/roadmap.ts (2)
ISortRoadmapRequestBody(67-71)IRoadmapPrivate(14-18)
⏰ 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). (7)
- GitHub Check: API / Integration Tests - OS ubuntu-24.04 - Node v22 - Postgres v18
- GitHub Check: API / Integration Tests - OS ubuntu-24.04 - Node v22 - Postgres v14
- GitHub Check: API / Integration Tests - OS ubuntu-24.04 - Node v22 - Postgres v17
- GitHub Check: API / Integration Tests - OS ubuntu-24.04 - Node v22 - Postgres v12.18
- GitHub Check: API / Integration Tests - OS ubuntu-24.04 - Node v22 - Postgres v16
- GitHub Check: test
- GitHub Check: lighthouse
| table.string("name", 50).notNullable(); | ||
| table.string("url", 50).notNullable().unique(); | ||
| table.integer("index").notNullable(); | ||
| table.string("index", 50).notNullable(); |
There was a problem hiding this comment.
Do not rewrite historical migration; provide a forward migration.
Changing the original 20201224104136 migration from integer to string(50) won’t touch already-provisioned databases. Those environments still have roadmaps.index as an INTEGER, so the new LexoRank strings will fail to insert (invalid input syntax for type integer). Please keep the original migration intact and add a new migration that alters the column type (and backfills existing rows) in place.
🤖 Prompt for AI Agents
packages/server/src/database/migrations/20201224104136_add-roadmaps-table.ts
lines ~10: do not modify this historical migration; instead create a new
migration file that alters roadmaps.index from integer to string(50) and
backfills existing rows. The new migration should run in a transaction, add a
temporary varchar(50) column, compute LexoRank strings for all existing integer
index values (and handle NULLs by assigning appropriate LexoRank positions),
write those strings into the temp column, migrate any constraints/indexes, drop
the old integer column, rename the temp column to index (preserving
notNullable), and in the down migration reverse the steps (convert the string
values back to integers or a safe default, restore original type and
constraints). Ensure the migration handles casting safely (no direct ALTER
TYPE), preserves existing data, and is idempotent for already-provisioned DBs.
Comment on lines
+47
to
+60
| const lastRoadmap = await database | ||
| .select("index") | ||
| .from("roadmaps") | ||
| .orderBy("index", "desc") | ||
| .first(); | ||
|
|
||
| let nextIndex: string; | ||
|
|
||
| if (!lastRoadmap) { | ||
| nextIndex = LexoRank.middle().toString(); | ||
| } else { | ||
| const prevLexoRank = LexoRank.parse(lastRoadmap.index); | ||
| nextIndex = prevLexoRank.genNext().toString(); | ||
| } |
There was a problem hiding this comment.
Guard against invalid legacy roadmap indexes before parsing
If any existing roadmap still carries a null, empty or pre-LexoRank index (e.g. after the type change but before a data backfill), LexoRank.parse(lastRoadmap.index) will throw and the create flow will 500. We should treat an absent/invalid index as “no prior rank” and fall back to LexoRank.middle() (ideally logging the anomaly).
- if (!lastRoadmap) {
- nextIndex = LexoRank.middle().toString();
- } else {
- const prevLexoRank = LexoRank.parse(lastRoadmap.index);
- nextIndex = prevLexoRank.genNext().toString();
- }
+ const lastRoadmapIndex = lastRoadmap?.index;
+
+ if (!lastRoadmapIndex) {
+ nextIndex = LexoRank.middle().toString();
+ } else {
+ try {
+ nextIndex = LexoRank.parse(lastRoadmapIndex).genNext().toString();
+ } catch {
+ logger.warn({
+ message: `Invalid LexoRank index detected while creating roadmap: ${lastRoadmapIndex}`,
+ });
+ nextIndex = LexoRank.middle().toString();
+ }
+ }🤖 Prompt for AI Agents
In packages/server/src/ee/controllers/v1/roadmaps/create.ts around lines 47 to
60, guard against null/empty/legacy indexes before calling LexoRank.parse: if
lastRoadmap is missing or lastRoadmap.index is falsy or fails to parse as a
LexoRank, treat it as “no prior rank” and use LexoRank.middle() for nextIndex;
if parsing fails catch the error, fall back to LexoRank.middle(), and log the
anomaly (including the offending index and roadmap id/context) so we can
identify legacy data issues.
Comment on lines
+68
to
71
| id: string; | ||
| prevRoadmapId: string; | ||
| nextRoadmapId: string; | ||
| } |
There was a problem hiding this comment.
Make neighbour roadmap ids optional in the contract
The server schema now allows either prevRoadmapId or nextRoadmapId, but the type keeps both mandatory, forcing callers to cast and making valid payloads appear illegal. Loosen the shape so TypeScript reflects reality.
export interface ISortRoadmapRequestBody {
id: string;
- prevRoadmapId: string;
- nextRoadmapId: string;
+ prevRoadmapId?: string;
+ nextRoadmapId?: string;
}📝 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
| id: string; | |
| prevRoadmapId: string; | |
| nextRoadmapId: string; | |
| } | |
| export interface ISortRoadmapRequestBody { | |
| id: string; | |
| prevRoadmapId?: string; | |
| nextRoadmapId?: string; | |
| } |
🤖 Prompt for AI Agents
In packages/types/src/roadmap.ts around lines 68 to 71, the interface currently
requires both prevRoadmapId and nextRoadmapId even though the server allows
either to be absent; change their types to be optional (e.g., prevRoadmapId?:
string and nextRoadmapId?: string) so callers no longer need to cast and
TypeScript accurately represents the permitted payload shape.
Ja4V8s28Ck
force-pushed
the
feat/lexographical-ranking-for-roadmaps
branch
from
665a29c to
13dcba1
Compare
Contributor
Author
|
@mittalyashu If you have the chance, can you do a stress test on this implementation and see how it performs? |
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.
Implemented
n + 1indexing had.Backward Compatibility
Summary by CodeRabbit
New Features
Bug Fixes