feat: move boards between workspaces#458
Conversation
Automated Code Review — Cage Match EditionTwo AI reviewers independently analyzed this PR: MaxwellMergeSlam's Review 🤼Verdict: APPROVE Summary: A clean, well-architected feature that moves boards between workspaces with proper permission gates and data integrity — this PR gets it done without breaking a sweat. Tyler Durden: "It's only after we've lost everything that we're free to do anything." And it's only after we've properly cleared workspace-scoped card assignments that we're free to move a board anywhere. Findings:
The Good:
The Concerns:
Neo: "I know kung fu." This PR knows its schema. APPROVE. KelvinBitBrawler's ReviewVerdict: REQUEST_CHANGES Summary: This PR introduces a functional board movement feature, complete with proper permission checks and transactional integrity, but suffers from thermal inefficiency and a frosty oversight in slug generation. Findings:
The Good:
The Concerns:
Consensus & ResolutionMaxwell: APPROVE | Kelvin: REQUEST_CHANGES Kelvin caught two valid issues that Maxwell missed:
Both fixes addressed in commit |
Implements the "Move to workspace" feature (kanbn#344) allowing users to relocate a board and all its contents (lists, cards, labels, checklists, comments, activity) to a different workspace. Key design decisions: - Card member assignments are cleared on move (they reference workspace-scoped members that may not exist in the target workspace) - Comments and activity history are preserved (they reference global user IDs, not workspace members) - Slug conflicts in the target workspace are auto-resolved by appending a UID suffix - Permission model: requires board:edit in source workspace and board:create in target workspace - Templates and archived boards cannot be moved Co-Authored-By: Claude <noreply@anthropic.com>
Address review feedback: - Consolidate 3 separate board queries into a single findFirst() that fetches all needed fields (id, name, slug, type, isArchived, workspaceId, createdBy) - Fix slug fallback to use board.name instead of publicId for human-readable URLs Co-Authored-By: Claude <noreply@anthropic.com>
Guests typically lack board:create permission in the target workspace, so showing them as destinations leads to a confusing server rejection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Moves the inline board query from the move mutation into the repo layer, consistent with how every other board mutation fetches data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
10 test cases covering auth, validation, permissions, slug conflict resolution, and the happy path. Follows webhook.test.ts patterns. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2c2a349 to
66251df
Compare
GitHub will now auto-collapse compiled translation files (messages.json, messages.ts, messages.po) in PR diffs and exclude them from language stats. This makes PRs that touch i18n strings much easier to review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Hey @hjball — heads up, the diff stats on this PR look scary (50k+ lines) but that's almost entirely auto-generated locale files (18 of 24 changed files). I've added a
|
|
This is great @nickmeinhold - please could you remove the locale changes from this PR (it's all handled automatically on merge now) |
hjball
left a comment
There was a problem hiding this comment.
This should be good to go after the locale changes have been removed
Reverts locale file diffs and .gitattributes to match main, per review feedback. The locale changes were unrelated translation updates that inflated the PR diff. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nickmeinhold
left a comment
There was a problem hiding this comment.
Locale file changes removed — the PR diff is now just the feature code (20 files, ~1.7k lines). Ready for another look when you get a chance!
Accept main's locale files and auto-resolve board/index.tsx. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| * Fetches the board fields needed by the move mutation: | ||
| * identity, naming, type guards, and workspace ownership. | ||
| */ | ||
| export const getBoardForMove = async ( |
There was a problem hiding this comment.
Can we use the existing getBoardbyPublicId func instead? That should already filter out deletedAt records
There was a problem hiding this comment.
Good catch — getByPublicId is the wrong shape here (it pulls the full board with cards/labels/joins for the board-detail view). I kept getBoardForMove because it has the right column projection for this flow, and added the isNull(deletedAt) filter inline. Same effect: tombstoned boards now return null.
| ); | ||
|
|
||
| // Get target workspace | ||
| const targetWorkspace = await workspaceRepo.getByPublicId( |
There was a problem hiding this comment.
We should probably check this doesn't return deleted records too
There was a problem hiding this comment.
Done — guarded at the call site. workspaceRepo.getByPublicId doesn't filter deletedAt (legacy: same is true for ~14 other callers across workspace.ts, member.ts, webhook.ts, etc.). Extended its column projection to include deletedAt and added || targetWorkspace.deletedAt to the guard. The wider fix to make the repo treat deleted-as-not-found across all callers is a separate concern — happy to follow up in another PR if you'd prefer that pattern globally.
| const boardCards = await tx | ||
| .select({ id: cards.id }) | ||
| .from(cards) | ||
| .where(and(inArray(cards.listId, listIds), isNull(cards.deletedAt))); |
There was a problem hiding this comment.
We'll want to include all cards (even those already deleted) so we don't have rouge members assignments when restoring cards in future
There was a problem hiding this comment.
Right, that's the rogue-restore case. Removed the isNull(deletedAt) filters on both the lists query and the cards query in this loop, so member assignments are now cleared for every card under every list ever associated with this board — soft-deleted or not. Added a regression test for the soft-deleted target workspace path while in the area (board-move.test.ts now has 11 tests, all passing).
hjball
left a comment
There was a problem hiding this comment.
Looking great @nickmeinhold - just a few small comments to address otherwise this one lgtm
Per @hjball's review: locale compilation/translations are handled automatically on merge to main, so this PR shouldn't carry them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous removal commit used local main, which had drifted from upstream. Re-syncing to upstream/main so the PR carries no locale diff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hey @nickmeinhold, couple of bits needed before we can merge this:
|
Three changes addressing @hjball's review comments, all about the schema treating deletedAt as optional metadata while the move-board flow needs it as a load-bearing invariant. 1. getBoardForMove now filters isNull(deletedAt). Moving a tombstoned board has no defensible semantics. Replaces the implicit "the board exists in the table" check with an explicit "the board is not soft-deleted" check. 2. Move-flow's clearing of cardToWorkspaceMembers now spans every card under every list ever associated with this board, including soft-deleted ones. If we leave member assignments on a deleted card and that card is later restored, the assignments would resurrect rogue references to workspace members from the OLD workspace. Removed the isNull filters on both lists and cards in that loop. 3. Move-flow now refuses to move into a soft-deleted target workspace. workspaceRepo.getByPublicId did not previously project deletedAt; extended its column selection so the call-site guard in board.move can check it. (A wider fix to make the repo treat deleted-as-not-found across all 14+ callers is left for a separate PR — narrow scope here.) Plus one regression test: throws NOT_FOUND when target workspace is soft-deleted. All 11 board-move tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ween-workspaces # Conflicts: # apps/web/i18n.lock # apps/web/src/locales/de/messages.json # apps/web/src/locales/de/messages.ts # apps/web/src/locales/en/messages.ts # apps/web/src/locales/es/messages.json # apps/web/src/locales/es/messages.ts # apps/web/src/locales/fr/messages.json # apps/web/src/locales/fr/messages.ts # apps/web/src/locales/it/messages.json # apps/web/src/locales/it/messages.ts # apps/web/src/locales/nl/messages.json # apps/web/src/locales/nl/messages.ts # apps/web/src/locales/pl/messages.json # apps/web/src/locales/pl/messages.ts # apps/web/src/locales/pt-BR/messages.json # apps/web/src/locales/pt-BR/messages.ts # apps/web/src/locales/ru/messages.json # apps/web/src/locales/ru/messages.ts
…ween-workspaces # Conflicts: # apps/web/src/locales/de/messages.json # apps/web/src/locales/es/messages.json # apps/web/src/locales/fr/messages.json # apps/web/src/locales/it/messages.json # apps/web/src/locales/nl/messages.json # apps/web/src/locales/pl/messages.json # apps/web/src/locales/pt-BR/messages.json # apps/web/src/locales/ru/messages.json
…ween-workspaces # Conflicts: # apps/web/i18n.lock # apps/web/src/locales/de/messages.json # apps/web/src/locales/de/messages.ts # apps/web/src/locales/en/messages.ts # apps/web/src/locales/es/messages.json # apps/web/src/locales/es/messages.ts # apps/web/src/locales/fr/messages.json # apps/web/src/locales/fr/messages.ts # apps/web/src/locales/it/messages.json # apps/web/src/locales/it/messages.ts # apps/web/src/locales/nl/messages.json # apps/web/src/locales/nl/messages.ts # apps/web/src/locales/pl/messages.json # apps/web/src/locales/pl/messages.ts # apps/web/src/locales/pt-BR/messages.json # apps/web/src/locales/pt-BR/messages.ts # apps/web/src/locales/ru/messages.json # apps/web/src/locales/ru/messages.ts # packages/db/src/repository/workspace.repo.ts
|
Conflicts resolved and CI is green again. The PR went stale purely from upstream drift (main moved ~32 commits ahead with the partner/Stripe/subscriptions work), not from anything outstanding on the review. Merged latest
11/11 board-move tests passing, @kan/db and @kan/api typecheck clean, build check green. Ready for another look when you get a chance. |
Summary
Adds the ability to move a board (and all its contents) from one workspace to another. Closes #344.
board.moveAPI mutation with dual permission checks (source + target workspace)Design decisions
Per @hjball's comment about handling comments, activity, and assignees:
workspaceMembers.idusers.idusers.idPermission model
board:editpermission (or creator) in the source workspaceboard:createpermission in the target workspaceWhat moves
Everything: lists, cards, labels, checklists, checklist items, comments, activity, attachments. All of these reference the board (directly or transitively), not the workspace — so updating
board.workspaceIdis sufficient.What gets cleared
Only
_card_workspace_membersentries (card assignees), because they join through workspace-scoped member IDs. Users can reassign cards after the move.Files changed
packages/db/src/repository/board.repo.ts—moveToWorkspace()transactional functionpackages/api/src/routers/board.ts—board.movemutationapps/web/src/views/board/components/BoardDropdown.tsx— menu itemapps/web/src/views/board/components/MoveBoardForm.tsx— new componentapps/web/src/views/board/index.tsx— modal registrationapps/web/src/locales/— extracted + compiled translationsTest plan
board:createin target workspace cannot move🤖 Generated with Claude Code