refactor: package code organization wave 12 - #2362
Merged
Merged
Conversation
…, task-id-integrity Behavior-preserving package organization after wave 11: - app/api: git, workspace-files, provider-status, github/gitlab-import, run-audit, task-diff, agent-import-generation (legacy re-exports kept) - types/run-audit.ts + types/planner-intervention.ts peels from types.ts - rename remaining-ops-5 → task-store/task-id-integrity.ts - Ratchet legacy.ts line-count ceiling (~5665 → ~3339)
Contributor
📝 WalkthroughWalkthroughThe PR modularizes core audit and intervention types, reroutes task-store helper wiring, and extracts dashboard API clients for providers, imports, Git operations, terminals, workspaces, planning, run audits, reviews, and task diffs while preserving legacy re-exports. ChangesCore task-store and type organization
Dashboard API modularization
Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant ApiClient
participant Backend
participant EventSource
Dashboard->>ApiClient: Start planning or interview session
ApiClient->>Backend: POST session request
Backend-->>ApiClient: Return sessionId
Dashboard->>EventSource: Connect to session stream
EventSource-->>Dashboard: Deliver questions, summaries, and terminal events
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR reorganizes dashboard API and core code without intentional behavior changes. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (7): Last reviewed commit: "refactor: package code organization wave..." | Re-trigger Greptile |
Contributor
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 `@packages/dashboard/app/api/provider-status.ts`:
- Around line 332-404: Update fetchPaperclipStatus, fetchPaperclipCompanies, and
fetchPaperclipAgents to stop putting apiKey in URLSearchParams or request paths.
Pass apiKey through the API client’s supported request-body or header mechanism
instead, while retaining apiUrl and companyId as needed query parameters so
failed requests cannot expose the secret in originalUrl logs.
In `@packages/dashboard/app/api/run-audit.ts`:
- Around line 210-214: Update the error handler in the agent children request to
detect HTTP 404s via err.status === 404, while retaining a case-insensitive
message fallback for “not found”; continue returning an empty array for either
case and rethrow all other errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dbde5b7-ffc9-44cb-acc8-705693e79edb
📒 Files selected for processing (17)
packages/core/src/__tests__/agent-logs-backend-mode.test.tspackages/core/src/archive-db.tspackages/core/src/store.tspackages/core/src/task-store/task-id-integrity.tspackages/core/src/types.tspackages/core/src/types/planner-intervention.tspackages/core/src/types/run-audit.tspackages/dashboard/app/api/agent-import-generation.tspackages/dashboard/app/api/git.tspackages/dashboard/app/api/github-import.tspackages/dashboard/app/api/gitlab-import.tspackages/dashboard/app/api/legacy.tspackages/dashboard/app/api/provider-status.tspackages/dashboard/app/api/run-audit.tspackages/dashboard/app/api/task-diff.tspackages/dashboard/app/api/workspace-files.tsscripts/line-count-baseline.json
Prefer ApiRequestError.status === 404 over case-sensitive message matching when agent children are missing, with a case-insensitive not-found fallback.
5 tasks
## Summary Wave 13 of package code organization (stacks on #2362 / wave 12). ### Peels | New module | Parent | |---|---| | `app/api/planning.ts` | planning mode + agent onboarding streams | | `app/api/dev-server.ts` | dev-server detect/session/log/preview client | | `app/api/mission-interview.ts` | mission / milestone / slice interview streams | | `app/api/models-usage.ts` | models registry + usage | | `app/api/task-steer.ts` | steer / spec revise / rebuild / refine | | `app/api/ai-summarize.ts` | AI title summarization | | `task-store/task-artifacts-ops.ts` | rename of `remaining-ops-7` | Public paths stay stable via `legacy.ts` re-exports (`app/api.ts` → `legacy`). ### LOC - `legacy.ts` ~3342 → **~1408** (under the 2000-line hard cap; removed from grandfather baseline) ### Shims - `legacy.ts` → peels above (delete-when: dashboard imports domain modules) - `remaining-ops-7` → `task-artifacts-ops` (rename complete) ## Test plan - [x] eslint on peeled API modules + legacy - [x] dashboard app typecheck (wave13-related errors cleared) - [x] core typecheck - [x] `pnpm check:line-count` (baseline updated) - [ ] CI merge gate
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/dashboard/app/api/dev-server.ts (2)
78-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
DevServerConfigdeclaration.
DevServerConfigis already declared identically at Lines 51-58. TypeScript merges the two, so it compiles, but this is a copy/paste artifact from the peel and should be removed to avoid drift.♻️ Remove the duplicate
-export interface DevServerConfig { - selectedScript: string | null; - selectedSource: string | null; - selectedCommand: string | null; - previewUrlOverride: string | null; - detectedPreviewUrl: string | null; - selectedAt: string | null; -} - interface BackendDevServerCandidate {🤖 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/dashboard/app/api/dev-server.ts` around lines 78 - 85, Remove the duplicate DevServerConfig interface declaration near the later declaration, keeping the original DevServerConfig definition unchanged and preserving all existing references to it.
873-873: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove orphaned JSDoc leftover from the peel.
This trailing
/** Get the SSE stream URL for a planning session */has no following declaration — the planning stream helper moved toplanning.ts. Dead comment; drop it.🤖 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/dashboard/app/api/dev-server.ts` at line 873, Remove the orphaned trailing JSDoc comment for the planning session SSE stream URL from the dev-server module, leaving the surrounding code unchanged.
🤖 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/dashboard/app/api/ai-summarize.ts`:
- Around line 46-61: Update the response parsing in the AI summarization request
to use the shared api() helper from client.ts, or otherwise wrap JSON.parse in
try/catch and validate that the result is a non-null object with string-valued
error/title fields before accessing them. Preserve the existing status-specific
errors and ensure only a string title is returned to satisfy the Promise<string>
contract.
---
Nitpick comments:
In `@packages/dashboard/app/api/dev-server.ts`:
- Around line 78-85: Remove the duplicate DevServerConfig interface declaration
near the later declaration, keeping the original DevServerConfig definition
unchanged and preserving all existing references to it.
- Line 873: Remove the orphaned trailing JSDoc comment for the planning session
SSE stream URL from the dev-server module, leaving the surrounding code
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0a7a2c8-548b-4d56-8d79-cfcf3c37defa
📒 Files selected for processing (11)
packages/core/src/__tests__/agent-logs-backend-mode.test.tspackages/core/src/store.tspackages/core/src/task-store/task-artifacts-ops.tspackages/dashboard/app/api/ai-summarize.tspackages/dashboard/app/api/dev-server.tspackages/dashboard/app/api/legacy.tspackages/dashboard/app/api/mission-interview.tspackages/dashboard/app/api/models-usage.tspackages/dashboard/app/api/planning.tspackages/dashboard/app/api/task-steer.tsscripts/line-count-baseline.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/tests/agent-logs-backend-mode.test.ts
- packages/core/src/store.ts
- scripts/line-count-baseline.json
- packages/dashboard/app/api/legacy.ts
Comment on lines
+46
to
+61
| const data = JSON.parse(bodyText) as { title?: string; error?: string }; | ||
|
|
||
| if (!res.ok) { | ||
| const errorMessage = data.error || "Request failed"; | ||
| if (res.status === 400) { | ||
| throw new Error(`Invalid request: ${errorMessage}`); | ||
| } else if (res.status === 429) { | ||
| throw new Error(`Rate limit exceeded: ${errorMessage}`); | ||
| } else if (res.status === 503) { | ||
| throw new Error(`AI service temporarily unavailable: ${errorMessage}`); | ||
| } else { | ||
| throw new Error(errorMessage); | ||
| } | ||
| } | ||
|
|
||
| if (!data.title) { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate parsed API responses before accessing fields.
The cast on Line [46] provides no runtime validation: malformed JSON throws a raw SyntaxError, while valid null or non-object JSON can crash at Lines [49] or [61]. A non-string title can also violate the declared Promise<string> contract. Parse inside try/catch and validate the response shape before reading error or returning title.
Proposed fix
- const data = JSON.parse(bodyText) as { title?: string; error?: string };
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(bodyText);
+ } catch {
+ throw new Error("API returned invalid JSON response");
+ }
+ if (typeof parsed !== "object" || parsed === null) {
+ throw new Error("API returned invalid JSON response");
+ }
+ const data = parsed as { title?: unknown; error?: unknown };
...
- const errorMessage = data.error || "Request failed";
+ const errorMessage = typeof data.error === "string" ? data.error : "Request failed";
...
- if (!data.title) {
+ const title = data.title;
+ if (typeof title !== "string" || title.length === 0) {
throw new Error("API returned empty title");
}
- return data.title;
+ return title;The shared api() helper in packages/dashboard/app/api/client.ts already handles invalid JSON explicitly.
📝 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.
Suggested change
| const data = JSON.parse(bodyText) as { title?: string; error?: string }; | |
| if (!res.ok) { | |
| const errorMessage = data.error || "Request failed"; | |
| if (res.status === 400) { | |
| throw new Error(`Invalid request: ${errorMessage}`); | |
| } else if (res.status === 429) { | |
| throw new Error(`Rate limit exceeded: ${errorMessage}`); | |
| } else if (res.status === 503) { | |
| throw new Error(`AI service temporarily unavailable: ${errorMessage}`); | |
| } else { | |
| throw new Error(errorMessage); | |
| } | |
| } | |
| if (!data.title) { | |
| let parsed: unknown; | |
| try { | |
| parsed = JSON.parse(bodyText); | |
| } catch { | |
| throw new Error("API returned invalid JSON response"); | |
| } | |
| if (typeof parsed !== "object" || parsed === null) { | |
| throw new Error("API returned invalid JSON response"); | |
| } | |
| const data = parsed as { title?: unknown; error?: unknown }; | |
| if (!res.ok) { | |
| const errorMessage = typeof data.error === "string" ? data.error : "Request failed"; | |
| if (res.status === 400) { | |
| throw new Error(`Invalid request: ${errorMessage}`); | |
| } else if (res.status === 429) { | |
| throw new Error(`Rate limit exceeded: ${errorMessage}`); | |
| } else if (res.status === 503) { | |
| throw new Error(`AI service temporarily unavailable: ${errorMessage}`); | |
| } else { | |
| throw new Error(errorMessage); | |
| } | |
| } | |
| const title = data.title; | |
| if (typeof title !== "string" || title.length === 0) { | |
| throw new Error("API returned empty title"); | |
| } | |
| return title; |
🤖 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/dashboard/app/api/ai-summarize.ts` around lines 46 - 61, Update the
response parsing in the AI summarization request to use the shared api() helper
from client.ts, or otherwise wrap JSON.parse in try/catch and validate that the
result is a non-null object with string-valued error/title fields before
accessing them. Preserve the existing status-specific errors and ensure only a
string title is returned to satisfy the Promise<string> contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Behavior-preserving package code organization (wave 12), continuing after wave 11 (#2333).
app/api/legacy.tsinto focused modules with stable re-exports:git.ts— remotes, PR management, terminal sessions, git management (withRepoPathpreserved)workspace-files.ts— file browser + workspace file opsprovider-status.ts— auth/CLI and runtime provider statusgithub-import.ts/gitlab-import.ts— issue/PR import clientsrun-audit.ts— run-audit, timeline, org tree, task reviewtask-diff.ts— task diffs and commit associationsagent-import-generation.ts— agent import catalog + generationtypes.ts:types/run-audit.tstypes/planner-intervention.tstask-store/remaining-ops-5.ts→task-store/task-id-integrity.ts(call sites updated)legacy.tsceiling ~5665 → ~3339; baseline refreshedNo intentional behavior changes; public import paths via
app/api/@fusion/coreremain stable.Test plan
@fusion/coretypechecktsconfig.apptypecheck (wave12-related errors cleared; pre-existing playwright/plugin env noise unchanged vs main)pnpm check:line-count(baseline updated)Summary by CodeRabbit