Conversation
|
WalkthroughRemoves runtime dotenv loading from shared build-config and trims exported env to isProduction/isDevelopment/NODE_ENV; migrates tm-core from tsc to tsup with a new tsup.config.ts injecting TM_PUBLIC_* at build time; updates CI/typecheck scripts; adjusts webview terminal logging, cwd, and error handling; minor UI formatting changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant TSUP as tsup (tm-core)
participant DM as dotenv-mono
participant BC as baseConfig (build-config)
participant Bundler as Bundler
participant Dist as Dist
Dev->>TSUP: run build (tsup)
TSUP->>DM: dotenvLoad()
DM-->>TSUP: populate process.env (TM_PUBLIC_*)
TSUP->>TSUP: getBuildTimeEnvs() collects TM_PUBLIC_*
TSUP->>BC: import baseConfig (no runtime dotenv side-effects)
TSUP->>TSUP: merge baseConfig + entries + dts + outDir + env
TSUP->>Bundler: inject TM_PUBLIC_* (replace process.env.TM_PUBLIC_*)
Bundler-->>Dist: emit ESM entries with injected public envs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-07-18T17:14:29.399ZApplied to files:
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: 4
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/package.json (2)
7-7: Types are generated to dist but package.json still points to src.With
dts: true, ship the generated .d.ts from dist to avoid consumers parsing TS sources and to match runtimeimportpaths.Apply:
- "types": "./src/index.ts", + "types": "./dist/index.d.ts", "exports": { ".": { - "types": "./src/index.ts", + "types": "./dist/index.d.ts", "import": "./dist/index.js" }, "./auth": { - "types": "./src/auth/index.ts", + "types": "./dist/auth/index.d.ts", "import": "./dist/auth/index.js" }, "./storage": { - "types": "./src/storage/index.ts", + "types": "./dist/storage/index.d.ts", "import": "./dist/storage/index.js" }, "./config": { - "types": "./src/config/index.ts", + "types": "./dist/config/index.d.ts", "import": "./dist/config/index.js" }, "./providers": { - "types": "./src/providers/index.ts", + "types": "./dist/providers/index.d.ts", "import": "./dist/providers/index.js" }, "./services": { - "types": "./src/services/index.ts", + "types": "./dist/services/index.d.ts", "import": "./dist/services/index.js" }, "./errors": { - "types": "./src/errors/index.ts", + "types": "./dist/errors/index.d.ts", "import": "./dist/errors/index.js" }, "./logger": { - "types": "./src/logger/index.ts", + "types": "./dist/logger/index.d.ts", "import": "./dist/logger/index.js" }, "./types": { - "types": "./src/types/index.ts", + "types": "./dist/types/index.d.ts", "import": "./dist/types/index.js" }, "./interfaces": { - "types": "./src/interfaces/index.ts", + "types": "./dist/interfaces/index.d.ts", "import": "./dist/interfaces/index.js" }, "./utils": { - "types": "./src/utils/index.ts", + "types": "./dist/utils/index.d.ts", "import": "./dist/utils/index.js" } },Also applies to: 11-11, 15-15, 19-19, 23-23, 27-27, 31-31, 35-35, 39-39, 43-43, 47-47, 51-51
26-33: Add missing "./parser" export to packages/tm-core/package.jsontsup builds parser/index and src/parser/index.ts exists, but package.json exports do not expose "./parser", so consumers cannot import @tm/core/parser.
File: packages/tm-core/package.json
"./providers": { "types": "./src/providers/index.ts", "import": "./dist/providers/index.js" }, + "./parser": { + "types": "./src/parser/index.ts", + "import": "./dist/parser/index.js" + }, "./services": {
📜 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 (3)
packages/build-config/src/tsup.base.ts(1 hunks)packages/tm-core/package.json(2 hunks)packages/tm-core/tsup.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/auth/config.ts:0-0
Timestamp: 2025-09-02T21:51:41.383Z
Learning: In packages/tm-core/src/auth/config.ts, the BASE_DOMAIN configuration intentionally does not include runtime environment variable fallbacks like TM_BASE_DOMAIN or HAMSTER_BASE_URL. The maintainers prefer to keep these capabilities "hush hush" and undocumented, using only the build-time TM_PUBLIC_BASE_DOMAIN and the default value.
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
📚 Learning: 2025-08-03T12:13:33.875Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-08-03T12:13:33.875Z
Learning: Applies to package.json : Add and update test scripts in package.json to include test, test:watch, test:coverage, test:unit, test:integration, test:e2e, and test:ci
Applied to files:
packages/tm-core/package.json
📚 Learning: 2025-09-02T21:51:41.383Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/auth/config.ts:0-0
Timestamp: 2025-09-02T21:51:41.383Z
Learning: In packages/tm-core/src/auth/config.ts, the BASE_DOMAIN configuration intentionally does not include runtime environment variable fallbacks like TM_BASE_DOMAIN or HAMSTER_BASE_URL. The maintainers prefer to keep these capabilities "hush hush" and undocumented, using only the build-time TM_PUBLIC_BASE_DOMAIN and the default value.
Applied to files:
packages/tm-core/tsup.config.tspackages/build-config/src/tsup.base.ts
🧬 Code graph analysis (1)
packages/tm-core/tsup.config.ts (1)
packages/build-config/src/tsup.base.ts (2)
mergeConfig(84-101)baseConfig(23-45)
⏰ 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: Typecheck
🔇 Additional comments (5)
packages/tm-core/package.json (3)
56-57: Switch to tsup for build/dev looks good.Using tsup here aligns with the new per-package tsup config.
74-74: Use workspace protocol for internal package.Avoid
"*"for monorepo-local@tm/build-configto ensure deterministic linking.Apply:
- "@tm/build-config": "*", + "@tm/build-config": "workspace:*",⛔ Skipped due to learnings
Learnt from: Crunchyman-ralph PR: eyaltoledano/claude-task-master#1090 File: apps/extension/package.json:241-243 Timestamp: 2025-08-07T13:00:22.966Z Learning: In monorepos, local packages should use "*" as the version constraint in package.json dependencies, as recommended by npm. This ensures the local version from within the same workspace is always used, rather than attempting to resolve from external registries. This applies to packages like task-master-ai within the eyaltoledano/claude-task-master monorepo.
77-79: Verify repo-wide dotenv-mono and tsup versions are consistent.
Sandbox verification failed — run a repo-wide check and align dotenv-mono and tsup versions to prevent subtle build diffs.packages/build-config/src/tsup.base.ts (1)
13-17: env helper trimmed correctly; confirm no callers relied on removed TM_PUBLIC_ values.*
Automated verification couldn't scan the repo; run a repo-wide search for env.TM_PUBLIC_* and for imports/destructuring of env from @tm/build-config — if any usages exist, restore the public vars or update callers.packages/tm-core/tsup.config.ts (1)
1-6: dotenv-mono load is placed correctly.Loading before env collection ensures TM_PUBLIC_* are available.
| const getBuildTimeEnvs = () => { | ||
| const envs: Record<string, string> = {}; | ||
| for (const [key, value] of Object.entries(process.env)) { | ||
| if (key.startsWith('TM_PUBLIC_')) { | ||
| // Return the actual value, not JSON.stringify'd | ||
| envs[key] = value || ''; | ||
| } | ||
| } | ||
| return envs; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make missing TM_PUBLIC_ fail fast (optional).*
Returning empty strings can mask misconfig at build time. Consider validating required keys (e.g., TM_PUBLIC_BASE_DOMAIN per team conventions).
Example:
const getBuildTimeEnvs = () => {
const envs: Record<string, string> = {};
+ const required = ['TM_PUBLIC_BASE_DOMAIN']; // extend as needed
for (const [key, value] of Object.entries(process.env)) {
if (key.startsWith('TM_PUBLIC_')) {
envs[key] = value || '';
}
}
+ for (const k of required) {
+ if (!(k in envs) || envs[k] === '') {
+ throw new Error(`Missing required env: ${k}`);
+ }
+ }
return envs;
};This aligns with the preference to rely solely on TM_PUBLIC_* at build time.
📝 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.
| const getBuildTimeEnvs = () => { | |
| const envs: Record<string, string> = {}; | |
| for (const [key, value] of Object.entries(process.env)) { | |
| if (key.startsWith('TM_PUBLIC_')) { | |
| // Return the actual value, not JSON.stringify'd | |
| envs[key] = value || ''; | |
| } | |
| } | |
| return envs; | |
| }; | |
| const getBuildTimeEnvs = () => { | |
| const envs: Record<string, string> = {}; | |
| const required = ['TM_PUBLIC_BASE_DOMAIN']; // extend as needed | |
| for (const [key, value] of Object.entries(process.env)) { | |
| if (key.startsWith('TM_PUBLIC_')) { | |
| // Return the actual value, not JSON.stringify'd | |
| envs[key] = value || ''; | |
| } | |
| } | |
| for (const k of required) { | |
| if (!(k in envs) || envs[k] === '') { | |
| throw new Error(`Missing required env: ${k}`); | |
| } | |
| } | |
| return envs; | |
| }; |
🤖 Prompt for AI Agents
In packages/tm-core/tsup.config.ts around lines 8 to 17, the helper currently
substitutes missing TM_PUBLIC_* vars with empty strings which can hide
misconfiguration; update getBuildTimeEnvs to validate required TM_PUBLIC_ keys
(for example TM_PUBLIC_BASE_DOMAIN or other team-specified names), and if any
required key is missing or empty throw an error or call process.exit(1) with a
clear message listing the missing variables so the build fails fast; keep
returning actual env values (not JSON.stringify) for present keys and make the
set of required keys configurable or declared at the top of the file.
| entry: { | ||
| index: 'src/index.ts', | ||
| 'auth/index': 'src/auth/index.ts', | ||
| 'config/index': 'src/config/index.ts', | ||
| 'errors/index': 'src/errors/index.ts', | ||
| 'interfaces/index': 'src/interfaces/index.ts', | ||
| 'logger/index': 'src/logger/index.ts', | ||
| 'parser/index': 'src/parser/index.ts', | ||
| 'providers/index': 'src/providers/index.ts', | ||
| 'services/index': 'src/services/index.ts', | ||
| 'storage/index': 'src/storage/index.ts', | ||
| 'types/index': 'src/types/index.ts', | ||
| 'utils/index': 'src/utils/index.ts' | ||
| }, |
There was a problem hiding this comment.
Entry list includes parser but package exports don’t.
Either export ./parser in package.json (preferred) or remove this entry to avoid dead outputs. See suggested package.json diff above.
🤖 Prompt for AI Agents
In packages/tm-core/tsup.config.ts around lines 21 to 34, the tsup entry list
includes 'parser/index' but package.json does not export ./parser, producing a
dead output; either remove 'parser/index' from the tsup entry map or (preferred)
add an export entry for "./parser" in packages/tm-core/package.json that points
to the built output (and its types) — e.g. add "./parser": { "import":
"./dist/parser/index.js", "types": "./dist/parser/index.d.ts" } — then rebuild
to ensure the parser bundle is included in package exports.
| 'types/index': 'src/types/index.ts', | ||
| 'utils/index': 'src/utils/index.ts' | ||
| }, | ||
| format: ['esm'], |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant format override.
baseConfig already sets format: ['esm']. Drop duplication for clarity.
Apply:
- format: ['esm'],📝 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.
| format: ['esm'], |
🤖 Prompt for AI Agents
In packages/tm-core/tsup.config.ts around line 35, the config redundantly sets
format: ['esm'] even though baseConfig already defines format: ['esm']; remove
the duplicate format entry from this file so it inherits the value from
baseConfig, leaving no other changes to the config object.
| dts: true, | ||
| outDir: 'dist', | ||
| // Replace process.env.TM_PUBLIC_* with actual values at build time | ||
| env: getBuildTimeEnvs() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: also define NODE_ENV at build-time.
If any code checks process.env.NODE_ENV, inject it here for dead-code elimination; base no longer defines env.
Apply:
- env: getBuildTimeEnvs()
+ env: { NODE_ENV: process.env.NODE_ENV || 'development', ...getBuildTimeEnvs() }📝 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.
| env: getBuildTimeEnvs() | |
| env: { NODE_ENV: process.env.NODE_ENV || 'development', ...getBuildTimeEnvs() } |
🤖 Prompt for AI Agents
In packages/tm-core/tsup.config.ts around line 39, the build-time env injection
only uses getBuildTimeEnvs() and doesn't ensure NODE_ENV is defined; update the
config so NODE_ENV is injected (e.g. env: { ...getBuildTimeEnvs(), NODE_ENV:
process.env.NODE_ENV ?? 'production' }) or add NODE_ENV inside
getBuildTimeEnvs() to provide a string value for dead-code elimination during
bundling.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/extension/src/components/TaskDetails/TaskMetadataSidebar.tsx (1)
103-124: Replace setTimeout with request/response ack to clear loading state.Fire-and-forget + fixed delay can leave UI stuck or flickering on slow/error cases. Send a requestId and toggle state on the response/error.
const handleStartTask = () => { if (!currentTask || isStartingTask) { return; } setIsStartingTask(true); // Send message to extension to open terminal if (vscode) { - vscode.postMessage({ + const requestId = `openTerminal:${currentTask.id}:${Date.now()}`; + vscode.postMessage({ type: 'openTerminal', taskId: currentTask.id, - taskTitle: currentTask.title + taskTitle: currentTask.title, + requestId }); } - - // Reset loading state after a short delay - setTimeout(() => { - setIsStartingTask(false); - }, 500); };Outside this block (supporting snippet), add a one-time message handler:
useEffect(() => { const handler = (event: MessageEvent) => { const msg = event.data; if (msg?.type === 'response' && msg.requestId?.startsWith('openTerminal:')) { setIsStartingTask(false); } if (msg?.type === 'error' && msg.requestId?.startsWith('openTerminal:')) { setIsStartingTask(false); console.error('Failed to open terminal:', msg.error); } }; window.addEventListener('message', handler); return () => window.removeEventListener('message', handler); }, []);
♻️ Duplicate comments (4)
packages/tm-core/tsup.config.ts (4)
21-34: Ensure entries align with package exports (parser).Either export ./parser in package.json or drop the entry to avoid dead outputs.
Check and suggest:
#!/bin/bash set -euo pipefail pkg="packages/tm-core/package.json" jq '.exports' "$pkg" jq -e 'has("exports") and .exports | has("./parser")' "$pkg" || { echo "Add ./parser to exports or remove 'parser/index' from tsup entries." >&2 exit 1 }Example package.json patch (supporting, outside this file):
{ "exports": { + "./parser": { + "import": "./dist/parser/index.js", + "types": "./dist/parser/index.d.ts" + } } }
8-17: Fail fast on missing required TM_PUBLIC_ vars (e.g., TM_PUBLIC_BASE_DOMAIN).*Prevents silent empty-string injection that masks misconfig.
const getBuildTimeEnvs = () => { const envs: Record<string, string> = {}; + const required = ['TM_PUBLIC_BASE_DOMAIN']; // extend as needed for (const [key, value] of Object.entries(process.env)) { if (key.startsWith('TM_PUBLIC_')) { // Return the actual value, not JSON.stringify'd envs[key] = value || ''; } } + const missing = required.filter((k) => !envs[k]); + if (missing.length) { + throw new Error(`Missing required build-time env(s): ${missing.join(', ')}`); + } return envs; };
35-35: Drop redundant format override; baseConfig already sets ESM.- format: ['esm'],
39-39: Also inject NODE_ENV for dead-code elimination.- env: getBuildTimeEnvs() + env: { NODE_ENV: process.env.NODE_ENV || 'production', ...getBuildTimeEnvs() }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks)apps/extension/src/components/TaskDetails/TaskMetadataSidebar.tsx(3 hunks)apps/extension/src/components/TaskDetailsView.tsx(1 hunks)apps/extension/src/services/webview-manager.ts(1 hunks)packages/tm-core/tsup.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
📚 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 : Use consistent formatting for task files, include all task properties in text files, and format dependencies with status indicators.
Applied to files:
apps/extension/src/components/TaskDetails/TaskMetadataSidebar.tsxapps/extension/src/components/TaskDetailsView.tsx
📚 Learning: 2025-09-02T21:51:41.383Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/auth/config.ts:0-0
Timestamp: 2025-09-02T21:51:41.383Z
Learning: In packages/tm-core/src/auth/config.ts, the BASE_DOMAIN configuration intentionally does not include runtime environment variable fallbacks like TM_BASE_DOMAIN or HAMSTER_BASE_URL. The maintainers prefer to keep these capabilities "hush hush" and undocumented, using only the build-time TM_PUBLIC_BASE_DOMAIN and the default value.
Applied to files:
packages/tm-core/tsup.config.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Do not rely on environment variables for API keys in tests; set mock environment variables in test setup.
Applied to files:
packages/tm-core/tsup.config.ts
🧬 Code graph analysis (2)
apps/extension/src/components/TaskDetailsView.tsx (1)
apps/extension/src/components/TaskDetails/TaskMetadataSidebar.tsx (1)
TaskMetadataSidebar(21-340)
packages/tm-core/tsup.config.ts (1)
packages/build-config/src/tsup.base.ts (2)
mergeConfig(84-101)baseConfig(23-45)
🔇 Additional comments (2)
.github/workflows/ci.yml (1)
86-88: turbo:build script present and turbo.json exists — no CI change requiredRoot package.json defines "turbo:build" -> "turbo build"; turbo.json is present at the repository root.
apps/extension/src/components/TaskDetailsView.tsx (1)
205-214: LGTM — props and layout unchanged (formatting only).
| this.logger.log( | ||
| `Opening terminal for task ${data.taskId}: ${data.taskTitle}` | ||
| ); | ||
|
|
||
| try { | ||
| const terminal = vscode.window.createTerminal({ | ||
| name: `Task ${data.taskId}: ${data.taskTitle}`, | ||
| cwd: this.workspaceRoot | ||
| cwd: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath | ||
| }); | ||
| terminal.show(); | ||
| this.logger.info('Terminal created and shown successfully'); | ||
|
|
||
| this.logger.log('Terminal created and shown successfully'); | ||
| response = { success: true }; | ||
| } catch (error) { | ||
| this.logger.error('Failed to create terminal:', error); | ||
| response = { success: false, error: error.message }; | ||
| response = { | ||
| success: false, | ||
| error: error instanceof Error ? error.message : 'Unknown error' | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return errors via the standard error channel instead of embedding {success:false} in data.
Throw after logging so callers with a requestId receive a type:"error" message; avoids wrapper success:true with inner success:false ambiguity.
try {
const terminal = vscode.window.createTerminal({
name: `Task ${data.taskId}: ${data.taskTitle}`,
cwd: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath
});
terminal.show();
- this.logger.log('Terminal created and shown successfully');
- response = { success: true };
+ this.logger.log('Terminal created and shown successfully');
+ response = { success: true };
} catch (error) {
this.logger.error('Failed to create terminal:', error);
- response = {
- success: false,
- error: error instanceof Error ? error.message : 'Unknown error'
- };
+ throw error;
}📝 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.
| this.logger.log( | |
| `Opening terminal for task ${data.taskId}: ${data.taskTitle}` | |
| ); | |
| try { | |
| const terminal = vscode.window.createTerminal({ | |
| name: `Task ${data.taskId}: ${data.taskTitle}`, | |
| cwd: this.workspaceRoot | |
| cwd: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath | |
| }); | |
| terminal.show(); | |
| this.logger.info('Terminal created and shown successfully'); | |
| this.logger.log('Terminal created and shown successfully'); | |
| response = { success: true }; | |
| } catch (error) { | |
| this.logger.error('Failed to create terminal:', error); | |
| response = { success: false, error: error.message }; | |
| response = { | |
| success: false, | |
| error: error instanceof Error ? error.message : 'Unknown error' | |
| }; | |
| this.logger.log( | |
| `Opening terminal for task ${data.taskId}: ${data.taskTitle}` | |
| ); | |
| try { | |
| const terminal = vscode.window.createTerminal({ | |
| name: `Task ${data.taskId}: ${data.taskTitle}`, | |
| cwd: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath | |
| }); | |
| terminal.show(); | |
| this.logger.log('Terminal created and shown successfully'); | |
| response = { success: true }; | |
| } catch (error) { | |
| this.logger.error('Failed to create terminal:', error); | |
| throw error; | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
36-38: Use npm ci;--frozen-lockfileis not an npm flag.Current installs use
npm install --frozen-lockfile --prefer-offline. Replace withnpm ci(optionally keep--prefer-offline) for deterministic CI installs and to avoid unsupported flag usage.Apply:
- - name: Install dependencies - run: npm install --frozen-lockfile --prefer-offline + - name: Install dependencies + run: npm ci --prefer-offlineAlso applies to: 59-61, 82-84, 107-109
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
package.json
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Add and update test scripts in package.json to include test, test:watch, test:coverage, test:unit, test:integration, test:e2e, and test:ci
Files:
package.json
🧠 Learnings (2)
📓 Common learnings
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
📚 Learning: 2025-08-03T12:13:33.875Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-08-03T12:13:33.875Z
Learning: Applies to package.json : Add and update test scripts in package.json to include test, test:watch, test:coverage, test:unit, test:integration, test:e2e, and test:ci
Applied to files:
package.json
⏰ 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 (2)
package.json (1)
28-35: Test scripts set is complete per repo guidelines.
test,test:watch,test:coverage,test:unit,test:integration,test:e2e, andtest:ciare all present..github/workflows/ci.yml (1)
63-66: Switch to Turbo typecheck is correct—verify workspace coverage.
npm run turbo:typecheckis fine; just confirm all workspaces have atypecheckscript so this job isn’t a no-op.Reuse the script from the package.json comment to list missing
typecheckscripts.
| - name: Build | ||
| run: npm run build | ||
| run: npm run turbo:build | ||
| env: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Build via Turbo is consistent with the monorepo setup.
Good consolidation to npm run turbo:build. Ensure cache is effective (Turbo local/remote) for faster CI.
Optionally enable a remote cache (e.g., Turborepo Remote Cache) using a repo secret to speed builds across PRs.
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 86-88: the workflow switches to running
npm run turbo:build but doesn't configure Turbo caching; to enable effective
caching add steps to restore/save the turborepo cache (or use the official
actions/cache) and, if using a remote cache, set TURBO_TOKEN and any remote
cache URL via repository secrets; update the env for the build step to include
any required TURBO_TOKEN and configure the cache action keys to include
package-lock.json and turbo.json (or workspace-specific files) so builds are
cached and shared across runs/PRs.
| "dev": "tsup --watch", | ||
| "turbo:dev": "turbo dev", | ||
| "turbo:build": "turbo build", | ||
| "turbo:typecheck": "turbo typecheck", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
turbo:typecheck script aligns CI with workspace tasks—good change.
This matches the CI update to call npm run turbo:typecheck. Ensure each workspace exposes a typecheck script so Turbo actually has tasks to run.
Run to verify every package defines a typecheck script:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a --strip-cwd-prefix package.json | while read -r f; do
pkg="$(jq -r '.name // "(root)"' "$f")"
script="$(jq -r '.scripts.typecheck // empty' "$f")"
if [[ -z "$script" ]]; then
echo "MISSING: $pkg -> scripts.typecheck"
else
echo "OK: $pkg -> $script"
fi
doneLength of output: 213
turbo:typecheck added — action required: add missing workspace typecheck scripts
turbo:typecheck aligns CI with workspace tasks — good change. Verification shows these packages lack scripts.typecheck: docs, extension, task-master-ai. Add a scripts.typecheck entry in each package.json (example: tsc --noEmit for TS packages) so Turbo runs typecheck across all workspaces.
🤖 Prompt for AI Agents
In package.json around line 18, the new root script "turbo:typecheck" expects
each workspace to expose a scripts.typecheck but the workspaces docs, extension,
and task-master-ai currently lack it; open each package's package.json and add a
"typecheck" script (for TypeScript packages add "tsc --noEmit", or an
appropriate typechecker for non-TS packages) under "scripts", then commit those
changes so Turbo can run typechecks across all workspaces.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
Refactor
Chores
Fixes