feat: add api-storage improvements#1278
Conversation
- improve set-status by removing what storage its using - add context-validator - improve task mapper to better handle metadata if it exists - add dependency-fetcher class that separates the domain of dependency fetching separately.
|
WalkthroughSubtask IDs and types normalized to strings across CLI and core; CLI rendering and task table layout adjusted; TaskMapper metadata and dependency handling updated; added Supabase DependencyFetcher and repository refactor to fetch dependencies by display_id; new repository types and TaskMapper tests added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI
participant Repo as SupabaseTaskRepository
participant Auth as AuthManager
participant DB as Supabase
participant Dep as DependencyFetcher
participant Mapper as TaskMapper
CLI->>Repo: getTasks()
Repo->>Auth: getContext()
Auth-->>Repo: { briefId }
Repo->>DB: select tasks by briefId (+relations)
DB-->>Repo: Task rows
Repo->>Dep: fetchDependenciesWithDisplayIds(taskIds)
Dep->>DB: select task_dependencies join tasks(display_id)
DB-->>Dep: dependency rows (with display_id)
Dep-->>Repo: Map<task_id, display_id[]>
Repo->>Mapper: mapDatabaseTasksToTasks(rows, depMap)
Mapper-->>Repo: Mapped Tasks
Repo-->>CLI: Tasks
sequenceDiagram
autonumber
actor CLI as CLI
participant Repo as SupabaseTaskRepository
participant Auth as AuthManager
participant DB as Supabase
participant Dep as DependencyFetcher
participant Mapper as TaskMapper
CLI->>Repo: getTask(id)
Repo->>Auth: getContext()
Auth-->>Repo: { briefId }
Repo->>DB: select task by id & briefId (+subtasks)
DB-->>Repo: Task row + subtasks
Repo->>Dep: fetchDependenciesWithDisplayIds([taskId, ...subtaskIds])
Dep->>DB: dependency query with join
DB-->>Dep: dependencies with display_id
Dep-->>Repo: Map<task_id, display_id[]>
Repo->>Mapper: mapDatabaseTasksToTasks(rows, depMap)
Mapper-->>Repo: Task with dependencies
Repo-->>CLI: Task
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-26T19:05:47.555ZApplied to files:
⏰ 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). (1)
🔇 Additional comments (1)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tm-core/src/types/index.ts (1)
230-236: Allow string subtask IDs in the type guard
isSubtaskstill insists ontypeof subtask.id === 'number', so any subtask coming from API storage with a stringdisplay_idnow fails the guard and gets rejected downstream. Please accept both numeric and string IDs here to match the newSubtaskcontract.- typeof subtask.id === 'number' && + (typeof subtask.id === 'number' || typeof subtask.id === 'string') &&packages/tm-core/src/mappers/TaskMapper.ts (1)
98-123: Clarify deprecation notice.The deprecation notice correctly marks this method as legacy but doesn't specify what should be used instead or where the new implementation lives.
Update the deprecation notice to be more specific:
/** * Groups dependencies by task ID (legacy method for backward compatibility) - * @deprecated Use DependencyFetcher.fetchDependenciesWithDisplayIds instead + * @deprecated Use DependencyFetcher.fetchDependenciesWithDisplayIds from '@/repositories/supabase' instead */
♻️ Duplicate comments (1)
packages/tm-core/src/types/repository-types.ts (1)
18-26: Duplicate type definition (referenced earlier).This
DependencyWithDisplayIdinterface is identical to the one defined inpackages/tm-core/src/repositories/supabase/dependency-fetcher.ts(lines 5-10). As noted in the review of dependency-fetcher.ts, keeping this definition here and importing it there would be the correct approach since this is the types file.The resolution should be to keep this definition here and remove the duplicate from dependency-fetcher.ts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
apps/cli/src/commands/list.command.ts(2 hunks)apps/cli/src/commands/set-status.command.ts(0 hunks)apps/cli/src/ui/components/task-detail.component.ts(3 hunks)apps/cli/src/utils/ui.ts(2 hunks)packages/tm-core/src/entities/task.entity.ts(1 hunks)packages/tm-core/src/mappers/TaskMapper.ts(5 hunks)packages/tm-core/src/repositories/supabase/dependency-fetcher.ts(1 hunks)packages/tm-core/src/repositories/supabase/index.ts(1 hunks)packages/tm-core/src/repositories/supabase/supabase-task-repository.ts(8 hunks)packages/tm-core/src/storage/api-storage.ts(1 hunks)packages/tm-core/src/types/index.ts(1 hunks)packages/tm-core/src/types/repository-types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/cli/src/commands/set-status.command.ts
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-07-18T17:09:45.690Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:45.690Z
Learning: Applies to scripts/modules/dependency-manager.js : Allow numeric subtask IDs to reference other subtasks within the same parent
Applied to files:
apps/cli/src/ui/components/task-detail.component.tspackages/tm-core/src/entities/task.entity.tspackages/tm-core/src/types/index.ts
📚 Learning: 2025-07-18T17:09:45.690Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:45.690Z
Learning: Applies to scripts/modules/dependency-manager.js : Format task and dependency IDs consistently when adding dependencies
Applied to files:
apps/cli/src/ui/components/task-detail.component.tspackages/tm-core/src/mappers/TaskMapper.ts
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Represent task dependencies as arrays of task IDs
Applied to files:
apps/cli/src/ui/components/task-detail.component.tspackages/tm-core/src/repositories/supabase/dependency-fetcher.tspackages/tm-core/src/mappers/TaskMapper.ts
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Use string IDs with dot notation (e.g., "1.2") for subtask references
Applied to files:
apps/cli/src/ui/components/task-detail.component.ts
📚 Learning: 2025-07-18T17:09:45.690Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:45.690Z
Learning: Applies to scripts/modules/dependency-manager.js : Support both task and subtask dependencies in cycle detection
Applied to files:
apps/cli/src/ui/components/task-detail.component.tspackages/tm-core/src/mappers/TaskMapper.ts
📚 Learning: 2025-07-18T17:14:29.399Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tasks.mdc:0-0
Timestamp: 2025-07-18T17:14:29.399Z
Learning: Applies to scripts/modules/task-manager.js : Allow filtering tasks by status within the current tag context, handle subtask display in lists, and use consistent table formats.
Applied to files:
apps/cli/src/ui/components/task-detail.component.ts
📚 Learning: 2025-07-18T17:16:32.622Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/ui.mdc:0-0
Timestamp: 2025-07-18T17:16:32.622Z
Learning: Applies to scripts/modules/ui.js : Use cli-table3 for table rendering, include colored bold headers, and set appropriate column widths for readability
Applied to files:
apps/cli/src/utils/ui.ts
📚 Learning: 2025-09-26T19:05:47.555Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1252
File: packages/ai-sdk-provider-grok-cli/package.json:11-13
Timestamp: 2025-09-26T19:05:47.555Z
Learning: In the eyaltoledano/claude-task-master repository, internal tm/ packages use a specific export pattern where the "exports" field points to TypeScript source files (./src/index.ts) while "main" points to compiled output (./dist/index.js) and "types" points to source files (./src/index.ts). This pattern is used consistently across internal packages like tm/core and tm/ai-sdk-provider-grok-cli because they are consumed directly during build-time bundling with tsdown rather than being published as separate packages.
Applied to files:
packages/tm-core/src/repositories/supabase/index.ts
📚 Learning: 2025-07-18T17:14:29.399Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tasks.mdc:0-0
Timestamp: 2025-07-18T17:14:29.399Z
Learning: Applies to scripts/modules/task-manager.js : Subtasks must use consistent properties, maintain simple numeric IDs unique within the parent task, and must not duplicate parent task properties.
Applied to files:
packages/tm-core/src/entities/task.entity.tspackages/tm-core/src/types/index.ts
📚 Learning: 2025-09-25T22:08:11.075Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1248
File: packages/tm-core/src/storage/api-storage.ts:509-0
Timestamp: 2025-09-25T22:08:11.075Z
Learning: In API storage backend, subtasks have unique IDs (like "HAM-21") and use a separate parent_id column to establish parent-child relationships, rather than using dotted notation like file storage (e.g., "21.1").
Applied to files:
packages/tm-core/src/types/index.ts
🧬 Code graph analysis (4)
packages/tm-core/src/types/repository-types.ts (1)
packages/tm-core/src/types/database.types.ts (2)
Tables(413-436)Database(9-411)
packages/tm-core/src/repositories/supabase/dependency-fetcher.ts (1)
packages/tm-core/src/types/database.types.ts (1)
Database(9-411)
packages/tm-core/src/repositories/supabase/supabase-task-repository.ts (4)
packages/tm-core/src/repositories/supabase/dependency-fetcher.ts (1)
DependencyFetcher(15-75)packages/tm-core/src/types/database.types.ts (2)
Database(9-411)Json(1-7)packages/tm-core/src/types/repository-types.ts (2)
TaskWithRelations(9-16)TaskDatabaseUpdate(40-41)packages/tm-core/src/mappers/TaskMapper.ts (1)
TaskMapper(12-199)
packages/tm-core/src/mappers/TaskMapper.ts (2)
packages/tm-core/src/types/database.types.ts (1)
Tables(413-436)packages/tm-core/src/types/index.ts (1)
Task(57-82)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (8)
packages/tm-core/src/repositories/supabase/dependency-fetcher.ts (1)
50-74: LGTM with minor note.The data processing logic correctly handles null/undefined cases and safely extracts display_ids. The Map construction is efficient.
Minor note: Consider adding a comment explaining why entries without
task_idare skipped (line 60), as this could help future maintainers understand the data quality expectations.packages/tm-core/src/mappers/TaskMapper.ts (4)
6-10: LGTM!The
DependencyRowtype provides good backward compatibility by supporting both the olddepends_on_task_idformat and the newdepends_on_task.display_idformat.
18-30: LGTM!The backward-compatible approach to handling both Map and array formats for dependencies is well-implemented. The
instanceofcheck cleanly differentiates between the two formats.
63-68: LGTM!The refactoring to use
extractMetadataFieldimproves consistency and maintainability by centralizing metadata access logic.Also applies to: 82-87
72-72: Remove redundant nullish coalescing.The
?? undefinedoperator is redundant whencomplexityis alreadynumber | null. The nullish coalescing operator convertsnulltoundefined, but this conversion is unnecessary if the receiving property acceptsundefinedalready. TypeScript will handle thenumber | null→number | undefinedconversion automatically.Apply this diff to simplify:
- complexity: subtask.complexity ?? undefined + complexity: subtask.complexity || undefinedAnd similarly on line 92:
- complexity: dbTask.complexity ?? undefined, + complexity: dbTask.complexity || undefined,Note: Use
|| undefinedinstead of?? undefinedonly if you want to treat0as falsy. If0is a valid complexity value, keep?? undefinedor just usesubtask.complexitydirectly if the type signature allowsnull.Also applies to: 92-92
⛔ Skipped due to learnings
Learnt from: CR PR: eyaltoledano/claude-task-master#0 File: .cursor/rules/tasks.mdc:0-0 Timestamp: 2025-07-18T17:14:29.399Z Learning: Applies to scripts/modules/task-manager.js : Use AI to generate detailed subtasks within the current tag context, considering complexity analysis for subtask counts and ensuring proper IDs for newly created subtasks.packages/tm-core/src/types/repository-types.ts (3)
1-16: LGTM!The
TaskWithRelationsinterface correctly extends the base task type with an optional document relation, matching the database schema structure.
28-59: LGTM!The type definitions for
TaskMetadata,TaskDatabaseUpdate,TaskQueryConfig, andTaskFetchResultare well-structured and provide clear contracts for repository operations.
61-83: LGTM!The custom error classes follow best practices by:
- Extending the base Error class
- Setting the
nameproperty for better error identification- Including contextual information (field/value for TaskValidationError)
packages/tm-core/src/repositories/supabase/dependency-fetcher.ts
Outdated
Show resolved
Hide resolved
1 participant
What type of PR is this?
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
New Features
Bug Fixes / Compatibility
Chores
Tests