docs: add next-phase optimization roadmap from full codebase audit#360
docs: add next-phase optimization roadmap from full codebase audit#360
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…improvement plan Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new “next-phase” optimization roadmap document based on a full monorepo audit, intended to guide technical-debt reduction and quality improvements ahead of v5.0 stabilization.
Changes:
- Introduces a 7-phase, dependency-ordered improvement roadmap (errors, types, tests, logging, linting, TODOs, performance).
- Documents audit metrics across packages (Error/ObjectQLError usage,
anycounts, console usage, TODOs, test gaps). - Proposes CI quality gates (coverage thresholds, automated TCK, benchmark regression checks).
| | Category | Current State | Target State | | ||
| |----------|---------------|--------------| | ||
| | `any` type usage | ~330 instances across packages | < 50 (critical path zero) | | ||
| | Error handling | Mixed (`throw new Error` ~100, `ObjectQLError` ~35) | 100% `ObjectQLError` in all packages | |
There was a problem hiding this comment.
The markdown tables use a leading double pipe (|| ...) which renders an extra empty first column in most markdown renderers. Use a single leading pipe (| ...) consistently (and update the separator row) so tables display as intended throughout this doc.
| |----------|---------------|--------------| | ||
| | `any` type usage | ~330 instances across packages | < 50 (critical path zero) | | ||
| | Error handling | Mixed (`throw new Error` ~100, `ObjectQLError` ~35) | 100% `ObjectQLError` in all packages | | ||
| | Test coverage | 100% packages have tests, but tools layer is weak | Full coverage including CLI and scaffolding tools | |
There was a problem hiding this comment.
This claims "100% packages have tests", but this same document later lists packages with 0 test files (e.g., @objectql/create, driver-tck, protocol-tck, vscode-objectql). Please revise this finding to match the audit data (e.g., distinguish runtime packages vs tools, or state that several tool packages currently have no tests).
| | Test coverage | 100% packages have tests, but tools layer is weak | Full coverage including CLI and scaffolding tools | | |
| | Test coverage | All runtime packages have tests; several CLI/TCK/editor tool packages currently have 0 tests | Full coverage including CLI and scaffolding tools | |
| | `tools/cli/src/commands/repl.ts` | 83 | `// HACK: We need to construct a repository.` | Refactor REPL to use proper kernel initialization | | ||
| | `tools/cli/src/commands/new.ts` | 181 | `// TODO: Implement action logic` | Implement or remove scaffold placeholder | | ||
| | `tools/cli/src/commands/new.ts` | 196 | `// TODO: Implement before insert logic` | Implement or remove scaffold placeholder | | ||
| | `tools/cli/src/commands/new.ts` | 206 | `// TODO: Implement after insert logic` | Implement or remove scaffold placeholder | | ||
| | `tools/cli/src/commands/migrate.ts` | 36 | `// TODO: Implement migration logic` | Implement migration or document as planned | | ||
| | `tools/cli/src/commands/migrate.ts` | 49 | `// TODO: Implement rollback logic` | Implement rollback or document as planned | |
There was a problem hiding this comment.
The file paths in the TODO matrix don’t match the repo layout. For example these CLI files live under packages/tools/cli/src/commands/..., not tools/cli/src/commands/.... Update the paths so readers can navigate directly from the roadmap.
| | `tools/cli/src/commands/repl.ts` | 83 | `// HACK: We need to construct a repository.` | Refactor REPL to use proper kernel initialization | | |
| | `tools/cli/src/commands/new.ts` | 181 | `// TODO: Implement action logic` | Implement or remove scaffold placeholder | | |
| | `tools/cli/src/commands/new.ts` | 196 | `// TODO: Implement before insert logic` | Implement or remove scaffold placeholder | | |
| | `tools/cli/src/commands/new.ts` | 206 | `// TODO: Implement after insert logic` | Implement or remove scaffold placeholder | | |
| | `tools/cli/src/commands/migrate.ts` | 36 | `// TODO: Implement migration logic` | Implement migration or document as planned | | |
| | `tools/cli/src/commands/migrate.ts` | 49 | `// TODO: Implement rollback logic` | Implement rollback or document as planned | | |
| | `packages/tools/cli/src/commands/repl.ts` | 83 | `// HACK: We need to construct a repository.` | Refactor REPL to use proper kernel initialization | | |
| | `packages/tools/cli/src/commands/new.ts` | 181 | `// TODO: Implement action logic` | Implement or remove scaffold placeholder | | |
| | `packages/tools/cli/src/commands/new.ts` | 196 | `// TODO: Implement before insert logic` | Implement or remove scaffold placeholder | | |
| | `packages/tools/cli/src/commands/new.ts` | 206 | `// TODO: Implement after insert logic` | Implement or remove scaffold placeholder | | |
| | `packages/tools/cli/src/commands/migrate.ts` | 36 | `// TODO: Implement migration logic` | Implement migration or document as planned | | |
| | `packages/tools/cli/src/commands/migrate.ts` | 49 | `// TODO: Implement rollback logic` | Implement rollback or document as planned | |
| | `protocols/odata-v4/src/index.ts` | 1540 | `// TODO: Need to extract and store the created ID` | Implement batch operation ID tracking | | ||
| | `protocols/odata-v4/src/index.ts` | 1544 | `// TODO: Need to store the deleted record data` | Implement batch delete record tracking | | ||
| | `protocols/odata-v4/src/index.ts` | 1548 | `// TODO: Need to fetch and store previous values` | Implement batch update pre-fetch | |
There was a problem hiding this comment.
Similarly, the OData TODO paths should include the packages/ prefix (e.g., packages/protocols/odata-v4/src/index.ts). As written, protocols/odata-v4/... won’t resolve in this repo.
| | `protocols/odata-v4/src/index.ts` | 1540 | `// TODO: Need to extract and store the created ID` | Implement batch operation ID tracking | | |
| | `protocols/odata-v4/src/index.ts` | 1544 | `// TODO: Need to store the deleted record data` | Implement batch delete record tracking | | |
| | `protocols/odata-v4/src/index.ts` | 1548 | `// TODO: Need to fetch and store previous values` | Implement batch update pre-fetch | | |
| | `packages/protocols/odata-v4/src/index.ts` | 1540 | `// TODO: Need to extract and store the created ID` | Implement batch operation ID tracking | | |
| | `packages/protocols/odata-v4/src/index.ts` | 1544 | `// TODO: Need to store the deleted record data` | Implement batch delete record tracking | | |
| | `packages/protocols/odata-v4/src/index.ts` | 1548 | `// TODO: Need to fetch and store previous values` | Implement batch update pre-fetch | |
|
|
||
| The coding standard mandates `ObjectQLError` for all thrown errors. Currently, ~100 instances of `throw new Error` exist across the codebase, while only ~35 use `ObjectQLError`. | ||
|
|
||
| **Definition** (already in `@objectql/types/src/api.ts`): |
There was a problem hiding this comment.
This references @objectql/types/src/api.ts, but in-repo the source lives at packages/foundation/types/src/api.ts (and consumers import from @objectql/types). Consider updating the reference to the actual repo path (or to the public module export) to avoid confusion for contributors following the roadmap.
| **Definition** (already in `@objectql/types/src/api.ts`): | |
| **Definition** (already exported from `@objectql/types`): |
| **Error Code Taxonomy** (new codes to define in `@objectql/types`): | ||
|
|
||
| ```typescript | ||
| // Proposed additions to ApiErrorCode | ||
| type DriverErrorCode = | ||
| | 'DRIVER_ERROR' | ||
| | 'DRIVER_CONNECTION_FAILED' | ||
| | 'DRIVER_QUERY_FAILED' | ||
| | 'DRIVER_TRANSACTION_FAILED' | ||
| | 'DRIVER_UNSUPPORTED_OPERATION'; | ||
|
|
||
| type ProtocolErrorCode = | ||
| | 'PROTOCOL_ERROR' | ||
| | 'PROTOCOL_INVALID_REQUEST' | ||
| | 'PROTOCOL_METHOD_NOT_FOUND' | ||
| | 'PROTOCOL_BATCH_ERROR'; | ||
|
|
||
| type PluginErrorCode = | ||
| | 'TENANT_ISOLATION_VIOLATION' | ||
| | 'TENANT_NOT_FOUND' | ||
| | 'WORKFLOW_TRANSITION_DENIED' | ||
| | 'FORMULA_EVALUATION_ERROR'; |
There was a problem hiding this comment.
The roadmap proposes adding new error codes as separate type ...ErrorCode unions, but the current contract defines ApiErrorCode as an enum in packages/foundation/types/src/api.ts. To keep the plan actionable and consistent with the existing type shape, describe these as new ApiErrorCode enum members (or explicitly note a planned contract change from enum→union).
| **Error Code Taxonomy** (new codes to define in `@objectql/types`): | |
| ```typescript | |
| // Proposed additions to ApiErrorCode | |
| type DriverErrorCode = | |
| | 'DRIVER_ERROR' | |
| | 'DRIVER_CONNECTION_FAILED' | |
| | 'DRIVER_QUERY_FAILED' | |
| | 'DRIVER_TRANSACTION_FAILED' | |
| | 'DRIVER_UNSUPPORTED_OPERATION'; | |
| type ProtocolErrorCode = | |
| | 'PROTOCOL_ERROR' | |
| | 'PROTOCOL_INVALID_REQUEST' | |
| | 'PROTOCOL_METHOD_NOT_FOUND' | |
| | 'PROTOCOL_BATCH_ERROR'; | |
| type PluginErrorCode = | |
| | 'TENANT_ISOLATION_VIOLATION' | |
| | 'TENANT_NOT_FOUND' | |
| | 'WORKFLOW_TRANSITION_DENIED' | |
| | 'FORMULA_EVALUATION_ERROR'; | |
| **Error Code Taxonomy** (new codes to define in `@objectql/types` as `ApiErrorCode` enum members): | |
| ```typescript | |
| // Proposed additions to the existing enum ApiErrorCode in packages/foundation/types/src/api.ts | |
| // Grouped by concern; these are NOT separate union types, but enum members to be added. | |
| // | |
| // Driver-related errors | |
| // ApiErrorCode.DRIVER_ERROR | |
| // ApiErrorCode.DRIVER_CONNECTION_FAILED | |
| // ApiErrorCode.DRIVER_QUERY_FAILED | |
| // ApiErrorCode.DRIVER_TRANSACTION_FAILED | |
| // ApiErrorCode.DRIVER_UNSUPPORTED_OPERATION | |
| // | |
| // Protocol-related errors | |
| // ApiErrorCode.PROTOCOL_ERROR | |
| // ApiErrorCode.PROTOCOL_INVALID_REQUEST | |
| // ApiErrorCode.PROTOCOL_METHOD_NOT_FOUND | |
| // ApiErrorCode.PROTOCOL_BATCH_ERROR | |
| // | |
| // Plugin / multi-tenant / workflow errors | |
| // ApiErrorCode.TENANT_ISOLATION_VIOLATION | |
| // ApiErrorCode.TENANT_NOT_FOUND | |
| // ApiErrorCode.WORKFLOW_TRANSITION_DENIED | |
| // ApiErrorCode.FORMULA_EVALUATION_ERROR |
Full codebase scan across all 30+ packages to quantify technical debt and produce an actionable improvement roadmap before v5.0 stabilization.
Audit findings
throw new Error(non-compliant)ObjectQLErroranytype usageconsole.*in prod sourceRoadmap (
docs/ROADMAP_NEXT_PHASE.md)7 phases, dependency-ordered:
Error→ObjectQLErrorwith semantic codes) +anyreduction. Priority packages: core (103 any, 30 Error), plugin-multitenancy (18 Error), driver-redis (10 Error)console.*from drivers/protocols, addno-consoleESLint ruleprefer-const→no-unused-vars→no-explicit-anysideEffects: false, core benchmark baselinesPhases 1–3 can parallelize. Phases 4–5 depend on Phase 1. Recommended to execute during Q1 P3 alongside existing housekeeping plan.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.