feat(boards): allow addItem to target a specific section - #6458
feat(boards): allow addItem to target a specific section#6458angusmaul wants to merge 3 commits into
Conversation
The addItem endpoint (OpenAPI POST /api/boards/items and MCP tool
board_addItem) always placed new items in the first empty section,
making it impossible for API clients to add items to named category
sections. This adds an optional sectionId to addItemToBoardSchema
which places the item at the next free grid position of the given
empty or category section, and a getSections query (OpenAPI GET
/api/boards/{boardId}/sections and MCP tool board_getSections) so
API clients can discover section ids. Without sectionId the previous
behaviour is unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚨 Preview Deployment Blocked - Security ProtectionYour pull request was blocked from triggering preview deployments Why was this blocked?
How to resolve this:Option 1: Get Collaborator Access (Recommended) Option 2: Request Permission Override For Repository Administrators:To disable this security check ( This security measure protects against malicious code execution in preview deployments. Only trusted collaborators should have the ability to trigger deployments. 🛡️ Learn more about this security featureThis protection prevents unauthorized users from:
Preview deployments are powerful but require trust. Only users with repository write access can trigger them. |
📝 WalkthroughWalkthroughThe board router adds section listing and section-aware item placement. Items may target a specified section or the first empty section, with validation for missing and dynamic sections. The validation schema, placement persistence, MCP description, and tests are updated. ChangesBoard section support
Sequence Diagram(s)sequenceDiagram
participant Client
participant boardRouter
participant BoardDatabase
Client->>boardRouter: addItem(boardId, sectionId?)
boardRouter->>BoardDatabase: resolve target section
BoardDatabase-->>boardRouter: target section
boardRouter->>BoardDatabase: insert item layout for target section
boardRouter-->>Client: created item
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/api/src/router/board.ts (1)
1452-1464: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReorder section-authority checks to avoid duplicate board queries.
throwIfActionForbiddenAsyncalready performs its ownboards.findFirst(..., columns: {id, creatorId, isPublic, userPermissions, groupPermissions}), so proceeding to the localboard = await ctx.db.query.boards.findFirst(..., with: { sections })double-fetches every board. Move the full-section query before that final authorization call, or pass the checked board into authorization. TheNOT_FOUNDcontract is already consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/router/board.ts` around lines 1452 - 1464, Reorder the board lookup and authorization flow in the query handler containing throwIfActionForbiddenAsync: fetch the board with its sections first, preserve the existing NOT_FOUND response, then perform the final authorization check without triggering a second board query, or reuse the fetched board through an appropriate authorization API. Keep the existing view permission and board ID checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/router/board.ts`:
- Around line 1436-1437: Update the MCP descriptions in
packages/api/src/router/board.ts at lines 1436-1437 and 1480-1482: document that
getSections requires view access and that boardId is obtained from
board_getAllBoards; document that addItem requires modify access and explain how
callers obtain any optional integration IDs.
In `@packages/validation/src/board.ts`:
- Line 127: Reject empty section IDs in the sectionId schema in
packages/validation/src/board.ts, while continuing to allow omitted values. In
packages/api/src/router/board.ts, update the related branching to distinguish
sectionId === undefined from provided values rather than using truthiness. Add a
regression test in packages/api/src/router/test/board.spec.ts covering
sectionId: "" and asserting it is rejected.
---
Nitpick comments:
In `@packages/api/src/router/board.ts`:
- Around line 1452-1464: Reorder the board lookup and authorization flow in the
query handler containing throwIfActionForbiddenAsync: fetch the board with its
sections first, preserve the existing NOT_FOUND response, then perform the final
authorization check without triggering a second board query, or reuse the
fetched board through an appropriate authorization API. Keep the existing view
permission and board ID checks intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9092f8f3-a8f1-4ba2-a472-d5207b5d2fbd
📒 Files selected for processing (3)
packages/api/src/router/board.tspackages/api/src/router/test/board.spec.tspackages/validation/src/board.ts
Reject empty sectionId at the input boundary and branch on undefined instead of truthiness, avoid a duplicate board query in getSections by relying on the access check for existence, and document permission requirements and id discovery in the MCP descriptions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ajnart
left a comment
ajnart
left a comment
There was a problem hiding this comment.
Thanks for the PR, I understand its goal I'm just not sure that this is how I would've gone about it. Please check my other comments
| const targetSection = | ||
| input.sectionId !== undefined | ||
| ? board.sections.find((section) => section.id === input.sectionId) | ||
| : board.sections | ||
| .filter((section) => section.kind === "empty") | ||
| .toSorted((sectionA, sectionB) => (sectionA.yOffset ?? 0) - (sectionB.yOffset ?? 0)) | ||
| .at(0); | ||
|
|
||
| if (!targetSection) { | ||
| throw new TRPCError( | ||
| input.sectionId !== undefined | ||
| ? { code: "NOT_FOUND", message: "Section not found on this board" } | ||
| : { code: "BAD_REQUEST", message: "Board has no empty section to place items in" }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This code looks really difficult to read with the ternaries, I'd recommend refactoring it
| const oldmarr = oldmarrConfigSchema.parse(JSON.parse(content)); | ||
| await importOldmarrAsync(ctx.db, oldmarr, input.configuration); | ||
| }), | ||
| getSections: protectedProcedure |
There was a problem hiding this comment.
I don't see the point of adding a whole procedure for this, instead consider making it a part of the endpoint to get a dashboard ?
I think overall my MCP implementation is not the best because we have many tools instead of having a few really powerful ones. In that case I believe it's beneficial to merge this behavior into the main dashboard related query
Remove the getSections procedure and instead include the sections of each board (id, kind, name, position) in getAllBoards, so section ids are discoverable without an additional tool. Extract the target section lookup of addItem into a findTargetSectionOrThrow helper to replace the hard to read ternaries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the review @ajnart — both points addressed:
Locally pnpm typecheck passes across all 37 packages and the board router suite is green (51 tests). |
3 participants
Description
The
board.addItemendpoint (OpenAPIPOST /api/boards/itemsand MCP toolboard_addItem) always places new items in the first empty section of a board. For boards organised into named category sections there is no API-reachable way to add an item to a specific category:addItemcan't target one, andsaveBoard(which can reposition items) is not exposed via OpenAPI or MCP. The only workaround is editing the database directly.This PR adds:
sectionIdtoaddItemToBoardSchema. When provided, the item is placed at the next free grid position of that section (the existing placement algorithm, unchanged — just pointed at the given section). Onlyemptyandcategorysections are allowed; adynamicsection id is rejected withBAD_REQUEST, and an unknown id withNOT_FOUND. When omitted, behaviour is exactly as before (first empty section), so existing OpenAPI/MCP clients are unaffected.id,kind,name, position) in thegetAllBoardsresponse, so API/MCP clients can discover thesectionIdto pass without an additional tool (per review feedback, instead of a separategetSectionsprocedure).getAllBoards).Motivation: automation and agent clients (e.g. via the built-in MCP server) managing a categorised dashboard — "add an app tile for service X in the Local Apps category".
Verified locally:
pnpm typecheck(all 37 packages) and the board router test suite (51 tests, including 6 new) pass; changed files formatted withoxfmt.Checklist
pnpm build, autofix withpnpm format:fix)devbranchx,y,ior any abbrevation)🤖 Generated with Claude Code