feat: User submit roadmap ideas #101
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
|
Warning Rate limit exceeded@arjunkomath has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughA new roadmap triage feature is introduced, enabling visitors to submit ideas for roadmap boards before formal inclusion. Implementation includes: database table for triage items, submission and management API endpoints, React components for modal submission and item display, type definitions, and integration into existing roadmap pages. Error handling across 20+ API routes is also improved by replacing Changes
Sequence DiagramsequenceDiagram
participant User as Visitor
participant PageApp as Page App
participant API1 as /api/roadmap/submit-triage
participant DB as Database
participant WebApp as Web App
participant API2 as /api/roadmap/triage/move-to-board
rect rgb(200, 220, 255)
Note over User,DB: Submit Triage Idea
User->>PageApp: Click "Contribute Idea"
PageApp->>PageApp: Open TriageSubmissionModal
User->>PageApp: Enter title/description
User->>PageApp: Submit
PageApp->>API1: POST with board_id, title, description
API1->>API1: Authenticate visitor
API1->>DB: Insert into roadmap_triage_items
DB-->>API1: Success
API1-->>PageApp: { success: true }
PageApp->>PageApp: Close modal
end
rect rgb(220, 255, 220)
Note over User,DB: Display & Move Triage Item
WebApp->>DB: Fetch triageItems for board
WebApp->>WebApp: Render TriageRow with items
User->>WebApp: Click "Move to Board"
WebApp->>API2: POST with triage_item_id
API2->>DB: Fetch triage item + board
API2->>DB: Get first column
API2->>DB: Find max position in column
API2->>DB: Insert roadmap_items (position+1)
API2->>DB: Delete triage_item
API2->>DB: Log audit entry
DB-->>API2: Success
API2-->>WebApp: { success: true, item: {...} }
WebApp->>WebApp: Update local state, remove from triage
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The PR introduces substantial new business logic with multiple interdependent components: database schema with migrations and RLS policies, three new authenticated API endpoints with complex position-conflict resolution and cascading deletes, multiple new React components requiring state coordination, type definitions across packages, and integration into existing pages. Additionally, the widespread (but repetitive) error-handling refactoring across 19 files adds scope. The move-to-board logic and authorization checks across API endpoints warrant careful review of correctness and security. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (14)
apps/web/pages/pages/[page_id]/roadmap/[board_id].tsx (1)
37-63: Optional: fold triage items into the board query to cut a round-trip.You can include a relation for roadmap_triage_items in the board select to reduce latency. Safe to defer.
apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx (2)
91-94: Replace alert with inline toast/snackbar.Alerts block UX and are easy to miss on mobile. Use your existing toast/notice component and auto-dismiss.
574-579: Lazy‑load the modal to trim initial bundle.Use next/dynamic for TriageSubmissionModal since many users won’t open it.
- import TriageSubmissionModal from "../../../../components/roadmap/TriageSubmissionModal"; + import dynamic from "next/dynamic"; + const TriageSubmissionModal = dynamic( + () => import("../../../../components/roadmap/TriageSubmissionModal") + );apps/page/pages/api/roadmap/submit-triage.ts (1)
10-22: Add basic spam/abuse controls.Consider per‑visitor rate limiting (e.g., sliding window in Redis) and CSRF protection for cookie‑based auth.
If you want, I can draft a small Redis-based limiter middleware compatible with Next API routes.
Also applies to: 31-37, 39-43
apps/page/components/roadmap/TriageSubmissionModal.tsx (1)
120-124: Improve a11y for error messaging.Add role and id; link inputs via aria-describedby when error exists.
- {error && ( - <div className="mb-4 p-3 bg-red-50 ..."> - <p className="text-sm text-red-600 ...">{error}</p> - </div> - )} + {error && ( + <div + id="triage-error" + role="alert" + className="mb-4 p-3 bg-red-50 ..." + > + <p className="text-sm text-red-600 ...">{error}</p> + </div> + )}Also add
aria-describedby={error ? "triage-error" : undefined}on fields as needed.apps/web/components/roadmap/TriageRow.tsx (2)
18-19: Sync local state when props change.Without syncing, external updates to triageItems won’t reflect locally.
- const [localTriageItems, setLocalTriageItems] = useState(triageItems); + const [localTriageItems, setLocalTriageItems] = useState(triageItems); + useEffect(() => { + setLocalTriageItems(triageItems); + }, [triageItems]);
31-35: Replace alerts with non-blocking notifications.Use your toast/notice system for consistency; keep API errors visible but unobtrusive.
Also applies to: 48-51
apps/web/pages/api/roadmap/triage/move-to-board.ts (1)
87-95: Make move-to-board atomic to avoid duplicates and racey positions.Insert + delete are not atomic; a delete failure leaves the triage item visible. Also, max(position)+1 can collide under concurrency.
Implement a Postgres function (RPC) that:
- locks the first column row,
- computes next position,
- inserts the item,
- deletes the triage row,
- returns the full inserted item,
all in a single transaction.Also applies to: 96-105, 58-68
apps/web/components/roadmap/RoadmapBoard.tsx (2)
52-76: Consider refining the position conflict resolution logic.The current implementation increments positions for all items with
position >= newItem.positionwhen a conflict is detected. This could unnecessarily shift multiple items. Consider these alternatives:
- Simpler approach: Assign the moved item the next available position:
- const hasPositionConflict = itemsInTargetColumn.some( - (item) => item.position === newItem.position - ); - - if (hasPositionConflict) { - const adjustedItems = itemsWithoutDuplicate.map((item) => { - if (item.column_id === newItem.column_id && item.position >= newItem.position) { - return { ...item, position: item.position + 1 }; - } - return item; - }); - return [...adjustedItems, newItem]; - } - - return [...itemsWithoutDuplicate, newItem]; + const maxPosition = itemsInTargetColumn.length > 0 + ? Math.max(...itemsInTargetColumn.map(item => item.position || 0)) + : -1; + const adjustedItem = { ...newItem, position: maxPosition + 1 }; + return [...itemsWithoutDuplicate, adjustedItem];
- Alternative: If preserving the server-assigned position is important, only increment items at the exact conflicting position rather than all items
>=.
78-79: Empty handler is acceptable but could add a comment.The empty
handleTriageItemDeletedhandler is fine sinceTriageRowmanages its own local state for triage items. Consider adding a comment to clarify this is intentional:const handleTriageItemDeleted = () => { + // No-op: TriageRow manages triage item removal locally };apps/web/components/roadmap/TriageItemCard.tsx (2)
36-46: Consider replacing nativealert()andconfirm()dialogs.The native
alert()andconfirm()dialogs work but provide a poor user experience—they're blocking, unstyleable, and can't be dismissed programmatically.Consider using a toast notification library (e.g.,
react-hot-toast,sonner) for error messages and a modal component for confirmations to maintain consistency with the rest of the application.Also applies to: 48-60
88-95: Add accessible label to the delete button.The delete button uses an icon without visible text. While the
titleattribute provides a tooltip, it's better to add anaria-labelfor screen readers:<button onClick={handleDelete} disabled={isMoving || isDeleting} className="px-3 py-2 bg-red-50 hover:bg-red-100 dark:bg-red-900/20 dark:hover:bg-red-900/30 text-red-600 dark:text-red-400 rounded-md transition-colors disabled:opacity-50 disabled:cursor-not-allowed" title="Delete" + aria-label="Delete submission" > <TrashIcon className="h-4 w-4" /> </button>packages/supabase/migrations/20_roadmap_triage_items.sql (2)
1-9: Consider adding rate limiting or duplicate prevention.The table allows unlimited submissions from the same visitor to a board. While this might be intentional for the feature, consider adding:
- Unique constraint to prevent exact duplicate submissions:
create unique index roadmap_triage_items_unique_submission on roadmap_triage_items(board_id, visitor_id, title, description) where description is not null; create unique index roadmap_triage_items_unique_submission_no_desc on roadmap_triage_items(board_id, visitor_id, title) where description is null;
- Application-level rate limiting to prevent spam (e.g., max 5 submissions per visitor per board per hour).
11-11: Consider adding an index onvisitor_idfor user-specific queries.If the application needs to query triage items by visitor (e.g., "show my submissions"), add an index:
create index roadmap_triage_items_visitor_id_idx on roadmap_triage_items(visitor_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/page/components/roadmap/TriageSubmissionModal.tsx(1 hunks)apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx(5 hunks)apps/page/pages/api/roadmap/submit-triage.ts(1 hunks)apps/web/components/roadmap/RoadmapBoard.tsx(3 hunks)apps/web/components/roadmap/TriageItemCard.tsx(1 hunks)apps/web/components/roadmap/TriageRow.tsx(1 hunks)apps/web/components/roadmap/types.ts(1 hunks)apps/web/pages/api/roadmap/triage/delete.ts(1 hunks)apps/web/pages/api/roadmap/triage/move-to-board.ts(1 hunks)apps/web/pages/pages/[page_id]/roadmap/[board_id].tsx(4 hunks)packages/supabase/migrations/20_roadmap_triage_items.sql(1 hunks)packages/supabase/types/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T09:03:10.349Z
Learnt from: arjunkomath
PR: techulus/changes-page#83
File: packages/supabase/types/index.ts:561-589
Timestamp: 2025-09-06T09:03:10.349Z
Learning: The roadmap_votes table in packages/supabase/migrations/18_roadmap.sql already has a unique_item_visitor constraint on (item_id, visitor_id) and the item_id foreign key includes ON DELETE CASCADE, so suggestions about adding these constraints are redundant.
Applied to files:
packages/supabase/migrations/20_roadmap_triage_items.sql
🧬 Code graph analysis (6)
apps/web/components/roadmap/RoadmapBoard.tsx (3)
packages/supabase/types/page.ts (3)
IRoadmapBoard(12-12)IRoadmapColumn(13-13)IRoadmapCategory(14-14)apps/web/components/roadmap/types.ts (1)
RoadmapItemWithRelations(7-10)apps/web/components/roadmap/TriageRow.tsx (1)
TriageRow(13-88)
apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx (1)
apps/page/components/roadmap/TriageSubmissionModal.tsx (1)
TriageSubmissionModal(13-148)
apps/page/pages/api/roadmap/submit-triage.ts (2)
apps/page/lib/visitor-auth.ts (1)
getAuthenticatedVisitor(92-110)packages/supabase/admin.ts (1)
supabaseAdmin(4-7)
apps/web/pages/api/roadmap/triage/delete.ts (3)
apps/web/utils/withAuth.ts (1)
withAuth(13-40)packages/supabase/admin.ts (1)
supabaseAdmin(4-7)apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/pages/api/roadmap/triage/move-to-board.ts (3)
apps/web/utils/withAuth.ts (1)
withAuth(13-40)packages/supabase/admin.ts (1)
supabaseAdmin(4-7)apps/web/utils/auditLog.ts (1)
createAuditLog(10-24)
apps/web/components/roadmap/TriageRow.tsx (2)
apps/web/components/roadmap/types.ts (1)
RoadmapItemWithRelations(7-10)apps/web/components/roadmap/TriageItemCard.tsx (1)
TriageItemCard(28-100)
🪛 Biome (2.1.2)
apps/page/pages/api/roadmap/submit-triage.ts
[error] 80-80: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
apps/web/pages/api/roadmap/triage/delete.ts
[error] 56-56: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
apps/web/pages/api/roadmap/triage/move-to-board.ts
[error] 111-111: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
🔇 Additional comments (6)
apps/web/components/roadmap/RoadmapBoard.tsx (2)
5-5: LGTM! Clean integration of triage functionality.The imports and prop definitions are well-structured. The optional
triageItemsprop with a default empty array is a good pattern.Also applies to: 10-10, 20-20, 26-26
83-89: LGTM! Proper conditional rendering.The triage row is correctly rendered only when items exist, and the callbacks are properly wired.
apps/web/components/roadmap/TriageItemCard.tsx (2)
11-26: LGTM! Well-implemented time formatting helper.The
getTimeAgofunction correctly validates the date input and provides appropriate fallbacks. The time ranges are clear and the implementation is straightforward.
2-2: No issues found—the import path is correct for your version.The codebase uses Heroicons v1.0.3 across
apps/webandapps/page, and the import path@heroicons/react/outlineis the correct syntax for v1.x. The import is valid as-is.Likely an incorrect or invalid review comment.
packages/supabase/types/index.ts (1)
591-635: LGTM! Type definition correctly mirrors the database schema.The
roadmap_triage_itemstable type is properly structured with:
- Correct field types matching the migration
- Appropriate optional fields in Insert/Update types
- Proper foreign key relationships with cascade deletes
- Consistent with the existing roadmap_items pattern
packages/supabase/migrations/20_roadmap_triage_items.sql (1)
27-30: LGTM! Standard timestamp trigger.The trigger for auto-updating
updated_atfollows the established pattern. Thetrigger_set_timestamp()function is defined in migration 0_schema.sql (lines 36-41), confirming it exists in an earlier migration and is available when migration 20_roadmap_triage_items.sql executes.
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/pages/api/integrations/zapier/trigger-new-post.tsx (1)
55-62: Add type guard before accessing error properties.Accessing
e.messageon anunknowntype at lines 56 and 61 requires type narrowing. This will cause TypeScript errors.Apply this diff to add proper type narrowing:
} catch (e: unknown) { - if (e.message.includes("Invalid")) { + const message = e instanceof Error ? e.message : 'An error occurred'; + if (message.includes("Invalid")) { return res .status(400) - .json({ error: { statusCode: 400, message: e.message } }); + .json({ error: { statusCode: 400, message } }); } - res.status(500).json({ error: { statusCode: 500, message: e.message } }); + res.status(500).json({ error: { statusCode: 500, message } }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/page/pages/api/json.ts(1 hunks)apps/page/pages/api/latest.ts(1 hunks)apps/page/pages/api/markdown.ts(1 hunks)apps/page/pages/api/notifications/subscribe-email.ts(1 hunks)apps/page/pages/api/pa/view.ts(1 hunks)apps/page/pages/api/pinned.ts(1 hunks)apps/page/pages/api/post/[id].ts(1 hunks)apps/page/pages/api/post/react.ts(1 hunks)apps/page/pages/api/post/reactions.ts(1 hunks)apps/page/pages/api/posts.ts(1 hunks)apps/page/pages/api/roadmap/submit-triage.ts(1 hunks)apps/page/pages/api/roadmap/vote.ts(3 hunks)apps/page/pages/api/roadmap/votes.ts(1 hunks)apps/page/pages/api/robots.ts(1 hunks)apps/page/pages/api/rss.ts(2 hunks)apps/page/pages/api/sitemap.ts(1 hunks)apps/web/pages/api/integrations/github/action-new-post.tsx(1 hunks)apps/web/pages/api/integrations/zapier/action-new-post.tsx(1 hunks)apps/web/pages/api/integrations/zapier/trigger-new-post.tsx(1 hunks)apps/web/pages/api/pages/reactions.ts(1 hunks)apps/web/pages/api/roadmap/triage/delete.ts(1 hunks)apps/web/pages/api/roadmap/triage/move-to-board.ts(1 hunks)apps/web/pages/pages/[page_id]/roadmap/[board_id].tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/pages/api/roadmap/triage/delete.ts
- apps/web/pages/api/roadmap/triage/move-to-board.ts
- apps/page/pages/api/roadmap/submit-triage.ts
- apps/web/pages/pages/[page_id]/roadmap/[board_id].tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: arjunkomath
PR: techulus/changes-page#101
File: packages/supabase/migrations/20_roadmap_triage_items.sql:15-25
Timestamp: 2025-10-18T23:41:55.486Z
Learning: In the changes-page codebase, roadmap triage items use service role (supabaseAdmin) for all database operations with authorization implemented at the API layer. The RLS policies on roadmap_triage_items are for page owner management only. Public INSERT happens through apps/page/pages/api/roadmap/submit-triage.ts which uses supabaseAdmin and checks board.is_public and visitor authentication. No INSERT RLS policy is needed.
🔇 Additional comments (14)
apps/page/pages/api/post/reactions.ts (1)
56-59: LGTM! Improved type safety.The change to
unknownfollows TypeScript best practices for error handling. The error is only logged and the response path is safe.apps/page/pages/api/sitemap.ts (1)
54-57: LGTM! Improved type safety.The change to
unknownfollows TypeScript best practices. The error handling is safe as it only logs the error without accessing its properties.apps/page/pages/api/markdown.ts (1)
42-45: LGTM! Improved type safety.The change to
unknownfollows TypeScript best practices. The error handling is safe as it only logs the error without accessing its properties.apps/page/pages/api/post/[id].ts (1)
70-73: LGTM! Improved type safety.The change to
unknownfollows TypeScript best practices. The error handling is safe as it only logs the error without accessing its properties.apps/page/pages/api/pinned.ts (1)
62-65: LGTM! Improved type safety.The change to
unknownfollows TypeScript best practices. The error handling is safe as it only logs the error without accessing its properties.apps/page/pages/api/json.ts (1)
45-48: LGTM! Improved type safety.The change to
unknownfollows TypeScript best practices. The error handling is safe as it only logs the error without accessing its properties.apps/page/pages/api/pa/view.ts (1)
55-58: LGTM! Good type safety improvement.The error type narrowing to
unknownimproves type safety. Since the error is only logged and not accessed for properties, this change is safe and aligns with TypeScript best practices.apps/page/pages/api/roadmap/votes.ts (1)
117-120: LGTM! Consistent error handling improvement.The error type narrowing to
unknownis safe here since the error is only logged without property access.apps/page/pages/api/robots.ts (1)
29-32: LGTM! Type-safe error handling.The change to
unknownis appropriate and safe for this error handling pattern.apps/web/pages/api/pages/reactions.ts (1)
41-44: LGTM! Safe error type narrowing.The change to
unknownis safe since only logging is performed without property access.apps/page/pages/api/post/react.ts (1)
44-47: LGTM! Appropriate type safety improvement.The error handling is correctly updated to use
unknownwith safe logging-only usage.apps/page/pages/api/latest.ts (1)
61-64: LGTM! Type-safe error handling.The narrowing to
unknownis safe and improves type safety without changing behavior.apps/page/pages/api/rss.ts (1)
1-4: LGTM: Improved error handling and import organization.The import consolidation at the top of the file improves code organization. The error handling change to
unknownis correctly implemented since the code doesn't access error properties, only logs the error and returns a generic message.Also applies to: 72-75
apps/page/pages/api/roadmap/vote.ts (1)
95-98: LGTM: Improved error handling type safety.The error handling change to
unknownis correctly implemented. The code logs the error and returns a generic message without accessing error properties, which is the proper way to handleunknownerror types.
1 participant
Summary by CodeRabbit
New Features