Throttle calls to calculate task folder size #2860
Closed
Conversation
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
🦋 Changeset detectedLatest commit: 2028dc7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dosubot
bot
added
size:M
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| try { | ||
| this.taskDirSize = await getFolderSize.loose(taskDir) | ||
| this.taskDirSizeCheckedAt = Date.now() | ||
| } catch (err) { |
Contributor
There was a problem hiding this comment.
When throttling the folder size calculation, if getFolderSize.loose(taskDir) fails (lines 392-400), the catch block logs the error but does not update taskDirSizeCheckedAt. This may result in repeated calls if the error persists. Consider updating taskDirSizeCheckedAt in a finally block (or even on error) to avoid continuous retries.
This comment was generated because it violated a code review rule: mrule_OR1S8PRRHcvbdFib.
mrubens
approved these changes
Apr 23, 2025
Collaborator
mrubens
left a comment
mrubens
left a comment
There was a problem hiding this comment.
Ellipsis comments make sense to me, but LGTM otherwise. Good find!
* feat: allow variable interpolation into the custom system prompt * fix: allow the test to pass on windows by using the path module
* fix: allow opening files without workspace root The openFile function in open-file.ts was requiring a workspace root to be present, which prevented opening global files (like MCP settings) when no workspace was open. Modified the function to handle absolute paths without this requirement. Previously, trying to open MCP settings in a new window without a workspace would error with "Could not open file: No workspace root found". Now the function properly handles both workspace-relative and absolute paths, allowing global settings files to be accessed in any context. Changes: - Removed workspace root requirement in openFile - Added fallback for relative paths when no workspace is present * fix: update openFile function to use provided path without modification --------- Co-authored-by: Roo Code <[email protected]> Co-authored-by: Matt Rubens <[email protected]>
) - Maintains editor view column state when closing and reopening files during diff operations, ensuring tabs stay opened in their original position. - Prevents closing the original editor tab when opening the diff view, preserving pinned status when applying changes via write_to_file or apply_diff. - Updates VSCode workspace launch flag from -n to -W for compatibility.
* feat: initialize VS Code Language Model client in constructor * feat: add VS Code LLM models and configuration * feat: integrate VS Code LLM models into API configuration normalization * Fix tests --------- Co-authored-by: Matt Rubens <[email protected]>
* feat: support environment variables reference in mcp `env` config * tests(src/utils/config): add test for `injectEnv` * fix(injectEnv): use `env == null` and `??` check instead of `!env`, `||` * refactor: remove unnecessary type declare * chore!: simplify regexp, remove replacement for env vars with dots
docs: update contributors list [skip ci] Co-authored-by: mrubens <[email protected]>
The FakeAI object passed by the user must be exactly the same object that is passed to FakeAIHandler via API configuration. Unfortunatelly, as the VSCode global state is used as configuration storage, we lose this property (VSCode global state creates copies of the object). Also the class of the stored object is lost, so methods of the object are unavailable. Therefore, we store the original objects in global variable and use ID field of FakeAI object to identify the original object.
* feat: Removed unnecessary cost calculation * Update vscode-lm.ts --------- Co-authored-by: Matt Rubens <[email protected]>
* Update validate.ts Allow ARNs from Bedrock Marketplace, which are different because models are deployed using SageMaker Inference behind the scenes. * Update bedrock.ts Allow ARNs from Bedrock Marketplace, which are different because models are deployed using SageMaker Inference behind the scenes.
* OpenRouter Gemini caching * Fix tests * Remove unsupported models * Clean up the task header a bit * Update src/api/providers/openrouter.ts Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * Remove model that doesn't seem to work --------- Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
* fix(chat): better loading feedback * fix(chat): missing loading aria role Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * refactor(chat): use vscode loading for more consistency --------- Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
* Format large numbers * Add changeset
* Package material icons in vsix * Add changeset
dosubot
bot
added
size:XXL
|
|
||
| let taskDirSize = 0 | ||
| if (Date.now() - this.taskDirSizeCheckedAt > this.taskDirSizeCheckInterval) { | ||
| this.taskDirSizeCheckedAt = Date.now() |
Contributor
There was a problem hiding this comment.
Setting taskDirSizeCheckedAt before the asynchronous folder size call may lead to a stale folder size when the call fails – subsequent calls won’t retry until after the full interval. Consider updating the timestamp only after a successful retrieval so that errors aren’t cached.
This comment was generated because it violated a code review rule: mrule_OR1S8PRRHcvbdFib.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Context
I was looking at what kind of performance win we'd get by using a YAML / append for our task files instead of JSON and noticed that we call this a lot more than we need to, and it's a pretty expensive call.
Implementation
Screenshots
How to Test
Get in Touch
Important
Throttle task folder size calculation and enhance environment variable handling, with UI improvements for model selection and task management.
Clineclass inCline.tsto reduce performance overhead.injectEnv()function inconfig.tsto replace environment variables in strings and objects.Mentioncomponent inMention.tsxfor handling text mentions.TaskActionscomponent inTaskActions.tsxfor task export and deletion.ContextWindowProgresscomponent inContextWindowProgress.tsxfor visualizing token usage.TaskHeader.tsxto use new components and improve task display.ModelPicker.tsxandApiOptions.tsxfor better model selection and configuration.injectEnv()inconfig.test.tsto ensure correct environment variable replacement.validate.tsto include new validation logic for API configurations.This description was created by
for 2028dc7. You can customize this summary. It will automatically update as commits are pushed.