Skip to content

Conversation

@hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Jul 1, 2025

Description

Fixed #5315

This PR moves task history from global state to workspace state, ensuring proper workspace isolation and fixing performance issues caused by shared global task history.

Changes Made

Core Architecture Changes

  • Moved taskHistory from globalSettingsSchema to new workspaceSettingsSchema in packages/types/
  • Created new WorkspaceSettings type and schema for workspace-specific settings
  • Extended ContextProxy to support workspace state management with new methods:
    • getWorkspaceState() and updateWorkspaceState() for workspace state access
    • getWorkspaceSettings() for typed workspace settings retrieval
    • Added workspace state cache and initialization

Migration Implementation

  • Added migrateTaskHistoryToWorkspace() function to migrate existing task history
  • Migration filters tasks by workspace path and moves them to appropriate workspace state
  • Preserves tasks from other workspaces in global state temporarily
  • Added comprehensive tests for migration scenarios

UI Updates

  • Removed "Show All Workspaces" toggle from History view
  • Removed workspace filtering logic from useTaskSearch hook
  • Updated tests to reflect workspace-isolated behavior
  • Removed workspace path display from TaskItem component

Provider Updates

  • Updated ClineProvider to use workspace state for task history operations
  • All task history reads/writes now use contextProxy.getWorkspaceState() and updateWorkspaceState()

Benefits

  1. Proper workspace isolation: Each workspace maintains its own task history
  2. Improved performance: Eliminates cross-window contention and "grey screen" crashes
  3. Better scalability: Can work with unlimited workspaces without degradation
  4. Cleaner architecture: Follows VSCode's intended state management patterns

Testing

  • All existing tests pass (508 passed, 1 skipped)
  • Added comprehensive tests for workspace state management in ContextProxy
  • Added migration tests covering various scenarios
  • Manual testing of task history in multiple workspaces
  • Verified migration works correctly for existing installations

Migration Strategy

The migration runs automatically on extension activation:

  1. Checks for existing task history in global state
  2. Filters tasks belonging to current workspace
  3. Moves them to workspace state
  4. Removes migrated tasks from global state
  5. Sets migration flag to prevent re-running

Breaking Changes

  • Removed "Show All Workspaces" feature - users can no longer view tasks from other workspaces
  • Task history is now workspace-specific and isolated

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Comments added for complex logic
  • Documentation updated
  • Breaking changes documented
  • All tests passing
  • Migration path implemented

- Remove workspace filtering in useTaskSearch hook to show all tasks
- Update tests to reflect global task history behavior
- Fix test expectations for sorting across multiple workspaces
- Remove unused cwd variable
Copilot AI review requested due to automatic review settings July 1, 2025 21:27
@hannesrudolph hannesrudolph requested review from cte, jr and mrubens as code owners July 1, 2025 21:27
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. bug Something isn't working labels Jul 1, 2025
@hannesrudolph hannesrudolph changed the title fix: update task search to show global history across all workspaces (#5315) fix: remove workspace filtering from task history search Jul 1, 2025
@delve-auditor
Copy link

delve-auditor bot commented Jul 1, 2025

No security or compliance issues detected. Reviewed everything up to 50b8476.

Security Overview
  • 🔎 Scanned files: 15 changed file(s)
Detected Code Changes

The diff is too large to display a summary of code changes.

Reply to this PR with @delve-auditor followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.

@hannesrudolph hannesrudolph changed the title fix: remove workspace filtering from task history search feat: Move taskHistory from globalState to workspaceState for workspace isolation Jul 1, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jul 1, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jul 1, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

This resolves the TypeScript compilation error where imports were resolving
to source files outside the configured rootDir
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jul 1, 2025
@hannesrudolph
Copy link
Collaborator Author

✅ All checks are now passing!

Fixed the TypeScript compilation error by correcting the import path in src/utils/migrateSettings.ts from a relative path to the proper package import @roo-code/types.

The PR is ready for review. All changes successfully refactor task history from global state to workspace state as requested in issue #5315.

- Update tests to pass raw context as first parameter
- Add tests for migration flag to prevent re-runs
- Fix error handling to catch all exceptions properly
- Remove obsolete context property from mockContextProxy
- Fix data loss issue: tasks without workspace property now migrate to current workspace
- Add error handling to workspace state operations with proper try-catch
- Make migration key workspace-specific to prevent race conditions
- Remove inappropriate console.log and global outputChannel usage
- Add workspace isolation notice UI component with translations
- Update tests to reflect new migration behavior
- Fixed critical bug where task.workspace was being mutated after updateWorkspaceState
- Added validation for currentWorkspacePath to prevent invalid migrations
- Improved error handling to prevent data loss if state updates fail
- Added test case to verify workspace property is correctly set on tasks
@daniel-lxs
Copy link
Member

Closing for now as it was a POC

@daniel-lxs daniel-lxs closed this Jul 2, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jul 2, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Prelim Review] to Done in Roo Code Roadmap Jul 2, 2025
@decentralize-all
Copy link

grok4 reviewed this + related issue & has some ideas:

Suggested Further Code Changes

Based on the PR's purpose—to isolate taskHistory in workspace state for better performance and to mitigate gray screen issues—I'll suggest improvements for each of the four changed files visible in the PR diffs. These focus on enhancing robustness, maintainability, test coverage, and edge-case handling. Since the PR is a closed POC with potentially more files (e.g., core implementation like migrateSettings.ts, UI components, or providers mentioned in the description but not shown in the extracted diffs), I'll note gaps at the end and suggest broader changes.

The suggestions assume TypeScript/VS Code extension context, emphasizing type safety, documentation, and migration reliability. I've included example diff-style code snippets for clarity where changes are substantive.

1. packages/types/src/global-settings.ts

This file removes taskHistory from the global schema, which is a good start for isolation. Improvements could include:

  • Adding JSDoc comments to explain the removal and its implications (e.g., migration needed).
  • Removing any lingering references or unused code (the diff already removes the import, but ensure no other history-related fields remain).
  • Adding a deprecation note or version field to the schema for future migrations.

Suggested Diff Excerpt:

@@ -7,7 +7,6 @@ import {
    providerSettingsEntrySchema,
    providerSettingsSchema,
} from "./provider-settings.js"
-  import { historyItemSchema } from "./history.js"
import { codebaseIndexModelsSchema, codebaseIndexConfigSchema } from "./codebase-index.js"
import { experimentsSchema } from "./experiment.js"
import { telemetrySettingsSchema } from "./telemetry.js"
@@ -26,7 +25,12 @@ export const globalSettingsSchema = z.object({
    lastShownAnnouncementId: z.string().optional(),
    customInstructions: z.string().optional(),
-      taskHistory: z.array(historyItemSchema).optional(),
    condensingApiConfigId: z.string().optional(),
    customCondensingPrompt: z.string().optional(),
+      /**
+       * @deprecated taskHistory moved to workspaceSettingsSchema for isolation (see PR #5321).
+       * Use workspace state instead. Migration handled in migrateSettings.
+       */
+      deprecatedTaskHistory: z.array(z.any()).optional().describe("Deprecated: Do not use."),
})

This adds a deprecated field temporarily to catch any old code references during transition, which can be removed in a follow-up PR.

2. packages/types/src/index.ts

This is a minimal export addition. Improvements are light:

  • Add a comment for context on why the new export is added.
  • Ensure alphabetical ordering of exports for consistency (if the repo follows that convention).

Suggested Diff Excerpt:

@@ -20,3 +20,5 @@ export * from "./terminal.js"
export * from "./tool.js"
export * from "./type-fu.js"
export * from "./vscode.js"
+  // Added for workspace-specific settings isolation (PR #5321)
+  export * from "./workspace-settings.js"

No major changes needed here, as it's straightforward.

3. packages/types/src/workspace-settings.ts (New File)

This defines the new workspace schema. To improve:

  • Expand JSDoc with examples and limits (e.g., max history size to prevent memory bloat causing gray screens).
  • Add validation refinements with Zod for history items (e.g., limit array length to 1000 to avoid performance issues).
  • Include a version field for future schema evolutions.

Suggested Full File Update Excerpt:

import { z } from "zod"
import { historyItemSchema } from "./history.js"

/**
 * WorkspaceSettings - Settings that are specific to a workspace.
 * This isolates data like taskHistory to prevent cross-workspace contention and improve performance.
 * @example
 * { taskHistory: [{ id: '1', ... }] }
 */
export const workspaceSettingsSchema = z.object({
    taskHistory: z.array(historyItemSchema)
        .optional()
        .refine((val) => !val || val.length <= 1000, {
            message: "Task history exceeds maximum length (1000 items) for performance reasons.",
        }),
    schemaVersion: z.number().optional().default(1), // For future migrations
})
export type WorkspaceSettings = z.infer<typeof workspaceSettingsSchema>
export const WORKSPACE_SETTINGS_KEYS = workspaceSettingsSchema.keyof().options
export type WorkspaceSettingsKey = keyof WorkspaceSettings

This adds safeguards against oversized histories, directly addressing the gray screen root cause.

4. src/tests/migrateSettings.spec.ts

This updates tests for migration with workspace handling. Improvements:

  • Add test cases for real migration scenarios: e.g., when global taskHistory exists, verify it's moved to workspace and cleared from global.
  • Include error handling tests (e.g., if workspace state update fails).
  • Test multi-workspace behavior (mock multiple contexts).
  • Use more realistic mock data for taskHistory.

Suggested Diff Excerpt (Expanding on Existing):

@@ -48,8 +50,18 @@ describe("Settings Migration", () => {
    mockContext = {
        globalStorageUri: { fsPath: mockStoragePath },
        globalState: {
            get: vitest.fn().mockReturnValue(undefined),
            update: vitest.fn().mockResolvedValue(undefined),
        },
    } as unknown as vscode.ExtensionContext
+      // Mock ContextProxy
+      mockContextProxy = {
+          getWorkspaceSettings: vitest.fn().mockReturnValue({ taskHistory: [] }),
+          updateWorkspaceState: vitest.fn().mockResolvedValue(undefined),
+      } as unknown as ContextProxy
    // Set global outputChannel for all tests
    ;(global as any).outputChannel = mockOutputChannel
})

+  it("migrates existing global taskHistory to workspace state", async () => {
+      mockContext.globalState.get.mockReturnValueOnce({ taskHistory: [{ id: 'test' }] });
+      await migrateSettings(mockContext, mockOutputChannel, mockContextProxy);
+      expect(mockContextProxy.updateWorkspaceState).toHaveBeenCalledWith('taskHistory', [{ id: 'test' }]);
+      expect(mockContext.globalState.update).toHaveBeenCalledWith('taskHistory', undefined); // Clear global
+  });
+
+  it("handles migration error gracefully", async () => {
+      mockContextProxy.updateWorkspaceState.mockRejectedValueOnce(new Error('Update failed'));
+      await expect(migrateSettings(mockContext, mockOutputChannel, mockContextProxy)).rejects.toThrow('Update failed');
+      expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(expect.stringContaining('Migration error'));
+  });

These tests ensure the migration is reliable and logs issues, preventing silent failures that could lead to persistent gray screens.

Broader Suggestions and Gaps

The visible changes are type/schema-focused, but the PR description mentions updates to core architecture, UI (e.g., removing "Show All Workspaces"), providers, and the actual migrateSettings.ts implementation. Since these aren't in the extracted diffs, potential gaps include:

  • Implementation Files (e.g., src/utils/migrateSettings.ts): Add logic to fetch global taskHistory, validate/parse with Zod, transfer to workspace via ContextProxy, and delete from global. Handle partial migrations (e.g., if workspace already has data, merge without duplicates).
  • UI/Components (e.g., chat/history views): Update any history-loading code to query workspace state only; add UI warnings for large histories.
  • Providers/Core (e.g., history providers): Refactor getters/setters to use workspaceState; implement async loading to avoid UI blocks.
  • General Improvements:
    • Add telemetry logging for migration success/failure.
    • Limit history size globally (e.g., auto-prune old items) to prevent future memory issues.
    • Integration tests for multi-window scenarios to verify no gray screens.
    • Documentation: Update README or contrib guide on the new state model.

If merged in a future PR, these would make the solution more complete. If you have access to the full repo, testing these locally could confirm fixes for your issue.

Analysis: Would This Fix the Gray Screen Issue?

  • Likely yes, at least partially. The gray screen you described often stems from overloaded state management, especially with large histories or multi-window usage, leading to extension host crashes or UI rendering failures. By isolating taskHistory to workspace state, the extension avoids accumulating global data that could bloat memory and cause contention (e.g., when scrolling long histories or running tasks across windows). This aligns with user-reported workarounds like clearing history to free up resources. The PR also implies simplifications like removing a "Show All Workspaces" toggle (mentioned in PR context, though not in the visible diffs), which could further reduce UI complexity and crash risks.
  • Potential limitations: Since it's a POC and closed, it might not cover all edge cases (e.g., migration of existing large histories or integration with other features). Related open issues suggest broader fixes like file-based storage might be needed for complete resolution. If you're comfortable, you could fork the repo, apply these changes locally, and test the extension build to see if it mitigates your issue.
  • Next steps: Monitor the repo for a merged version of this refactor (or a similar one). In the meantime, try the workarounds from before, like clearing history or restarting the extension. If the problem persists, report it on the issue tracker with your VS Code version, OS, and reproduction steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working PR - Needs Preliminary Review size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Refactor: Move taskHistory from globalState to workspaceState for proper workspace isolation

4 participants