fix(task): preserve labels on project import - #1519
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTask imports now accept optional labels and persist unique labels during task creation. Task exports and imports preserve labels. The delete-task route now validates entitlements before deletion. ChangesTask import and access control
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
PR Summary by QodoFix task import to preserve task labels on project import
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![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
🤖 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 `@apps/api/src/task/controllers/import-tasks.ts`:
- Around line 85-102: Update the label recreation logic around uniqueLabels to
trim or otherwise filter out labels whose names are empty or whitespace-only
before inserting. Use the filtered collection for both the insert values and
emptiness check, skipping the database insert when no valid labels remain.
In `@apps/api/src/task/index.ts`:
- Around line 483-486: Add the existing requireEntitlement middleware to the
single-task delete route before its async handler, matching the middleware
sequence used by other single-task mutation routes while preserving the existing
workspaceAccess and task delete permission checks.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8f518b9-c62f-42ff-832c-0c25ebcdcd5c
📒 Files selected for processing (2)
apps/api/src/task/controllers/import-tasks.tsapps/api/src/task/index.ts
| // Re-create the task's labels. export-tasks serialises `labels` per task, | ||
| // so without this an export/import round-trip silently drops them. | ||
| if (task && taskData.labels?.length) { | ||
| const uniqueLabels = [ | ||
| ...new Map(taskData.labels.map((label) => [label.name, label])).values(), | ||
| ]; | ||
|
|
||
| await tx | ||
| .insert(labelTable) | ||
| .values( | ||
| uniqueLabels.map((label) => ({ | ||
| name: label.name, | ||
| color: label.color, | ||
| taskId: task.id, | ||
| })), | ||
| ) | ||
| .onConflictDoNothing(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Filter out blank label names before insert.
The route validator accepts any string for name, including "" or whitespace. labelTable.name is notNull but not checked for content, so an import can persist blank labels. The layer contract states that only nonempty labels are inserted. Add the filter and skip the insert when nothing remains.
🛠️ Proposed fix
- if (task && taskData.labels?.length) {
- const uniqueLabels = [
- ...new Map(taskData.labels.map((label) => [label.name, label])).values(),
- ];
-
- await tx
- .insert(labelTable)
- .values(
- uniqueLabels.map((label) => ({
- name: label.name,
- color: label.color,
- taskId: task.id,
- })),
- )
- .onConflictDoNothing();
- }
+ if (task && taskData.labels?.length) {
+ const uniqueLabels = [
+ ...new Map(
+ taskData.labels
+ .map((label) => ({ ...label, name: label.name.trim() }))
+ .filter((label) => label.name.length > 0)
+ .map((label) => [label.name, label] as const),
+ ).values(),
+ ];
+
+ if (uniqueLabels.length > 0) {
+ await tx
+ .insert(labelTable)
+ .values(
+ uniqueLabels.map((label) => ({
+ name: label.name,
+ color: label.color,
+ taskId: task.id,
+ })),
+ )
+ .onConflictDoNothing();
+ }
+ }📝 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.
| // Re-create the task's labels. export-tasks serialises `labels` per task, | |
| // so without this an export/import round-trip silently drops them. | |
| if (task && taskData.labels?.length) { | |
| const uniqueLabels = [ | |
| ...new Map(taskData.labels.map((label) => [label.name, label])).values(), | |
| ]; | |
| await tx | |
| .insert(labelTable) | |
| .values( | |
| uniqueLabels.map((label) => ({ | |
| name: label.name, | |
| color: label.color, | |
| taskId: task.id, | |
| })), | |
| ) | |
| .onConflictDoNothing(); | |
| } | |
| // Re-create the task's labels. export-tasks serialises `labels` per task, | |
| // so without this an export/import round-trip silently drops them. | |
| if (task && taskData.labels?.length) { | |
| const uniqueLabels = [ | |
| ...new Map( | |
| taskData.labels | |
| .map((label) => ({ ...label, name: label.name.trim() })) | |
| .filter((label) => label.name.length > 0) | |
| .map((label) => [label.name, label] as const), | |
| ).values(), | |
| ]; | |
| if (uniqueLabels.length > 0) { | |
| await tx | |
| .insert(labelTable) | |
| .values( | |
| uniqueLabels.map((label) => ({ | |
| name: label.name, | |
| color: label.color, | |
| taskId: task.id, | |
| })), | |
| ) | |
| .onConflictDoNothing(); | |
| } | |
| } |
🤖 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 `@apps/api/src/task/controllers/import-tasks.ts` around lines 85 - 102, Update
the label recreation logic around uniqueLabels to trim or otherwise filter out
labels whose names are empty or whitespace-only before inserting. Use the
filtered collection for both the insert values and emptiness check, skipping the
database insert when no valid labels remain.
| validator("param", v.object({ id: v.string() })), | ||
| workspaceAccess.fromTask(), | ||
| requireWorkspacePermission({ task: ["delete"] }), | ||
| async (c) => { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare requireEntitlement usage across delete routes in the API.
set -euo pipefail
# List every route registration that references requireEntitlement or a delete permission.
rg -nP -C 6 'requireWorkspacePermission\(\{\s*\w+:\s*\[\s*"delete"' apps/api/src --type=ts
# Show all requireEntitlement call sites for comparison.
rg -nP '\brequireEntitlement\b' apps/api/src --type=tsRepository: usekaneo/kaneo
Length of output: 4966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== task/index.ts related snippets =="
sed -n '1,120p' apps/api/src/task/index.ts
sed -n '180,310p' apps/api/src/task/index.ts
sed -n '330,500p' apps/api/src/task/index.ts
sed -n '830,910p' apps/api/src/task/index.ts
echo "== billing middleware =="
cat -n apps/api/src/billing/require-entitlement-middleware.ts
echo "== require-task-permission.ts =="
cat -n apps/api/src/task/controllers/require-task-permission.ts
echo "== route registrations around existing requireEntitlement in task/index.ts =="
rg -n -C 8 'validator\("param"|validator\("json"|requireEntitlement|requireWorkspacePermission|requireBulkTaskEntitlement' apps/api/src/task/index.ts apps/api/src/task/controllers/require-task-permission.ts apps/api/src/project/index.ts apps/api/src/label/index.tsRepository: usekaneo/kaneo
Length of output: 50370
Add requireEntitlement to the single-task delete route.
The /tasks/:id delete handler only checks workspaceAccess and task: ["delete"], then calls deleteTask. Add the same entitlement middleware that protects single-task mutation routes so expired/cancelled workspaces cannot delete tasks.
🤖 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 `@apps/api/src/task/index.ts` around lines 483 - 486, Add the existing
requireEntitlement middleware to the single-task delete route before its async
handler, matching the middleware sequence used by other single-task mutation
routes while preserving the existing workspaceAccess and task delete permission
checks.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Imported labels were inserted without workspaceId, so workspaceAccess.fromLabel could not resolve their workspace (label endpoints failed with 'Workspace ID could not be determined') and bulk label removal, which filters by workspaceId, skipped them. Set it from the project being imported into.
|
@eeshsaxena thank you for your contribution! Please take a look at the suggestions from coderabbit and qodo. In addition, our lint step in CI is failing (specifically in Biome.) This should have thrown when you committed to the branch as well. |
2 participants
Problem
Exporting a project and importing it back silently loses every task's labels.
export-tasksserialises labels for each task:But
import-tasksnever reads them:ImportTaskhas nolabelsfield, the import route's validator strips the key, and the controller only inserts the task row. So an export -> import round-trip drops all labels.Fix
labels?: Array<{ name: string; color: string }>toImportTask.labelsin the/import/:projectIdroute validator (otherwise valibot strips it before the controller runs).Labels are de-duplicated by name before insert, and the insert uses
onConflictDoNothing()so the(task_id, name)unique constraint is respected.workspaceIdis left null, which is how task-scoped labels are already stored.The change is additive: exports made before this still import fine (labels default to none), and imports without a
labelsfield behave exactly as before.Summary by CodeRabbit
New Features
Bug Fixes