-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix: multiple cn nitpicks #8049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 issues found across 15 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="extensions/cli/src/ui/TextBuffer.ts">
<violation number="1" location="extensions/cli/src/ui/TextBuffer.ts:165">
deleteLineForward() returns without removing the newline when the cursor is at end-of-line, so ctrl+k/meta+Delete stop working there.</violation>
</file>
<file name="extensions/cli/src/util/yamlConfigUpdater.ts">
<violation number="1" location="extensions/cli/src/util/yamlConfigUpdater.ts:65">
By switching the filter to only exclude "anthropic/claude-sonnet-4-5", existing configs that still contain "anthropic/claude-4-sonnet" will keep that stale entry and then add the new model, so the migration fails to swap out the old slug and leaves duplicates.</violation>
</file>
<file name="extensions/cli/src/integration/model-persistence-unauthenticated.test.ts">
<violation number="1" location="extensions/cli/src/integration/model-persistence-unauthenticated.test.ts:209">
Updating CONTINUE_API_KEY in this test overwrites any existing value and the final delete removes it instead of restoring the original. If the variable was preset before the suite, this leaves later tests without the expected key. Please capture the prior value and restore it after the assertions.</violation>
</file>
<file name="extensions/cli/src/integration/model-persistence-e2e.test.ts">
<violation number="1" location="extensions/cli/src/integration/model-persistence-e2e.test.ts:103">
Rule violated: **Don't use console.log**
New console.log statements were added to this test, but the guideline requires using the structured logger instead of console.* calls. Please swap these console logs out for the approved logging utility (and apply the same change to the other new console.log lines mentioned in the evidence).</violation>
</file>
<file name="extensions/cli/src/integration/model-persistence-user-flow.test.ts">
<violation number="1" location="extensions/cli/src/integration/model-persistence-user-flow.test.ts:95">
Rule violated: **Don't use console.log**
Please replace these new console.log statements with the approved logger API to comply with the "Don't use console.log" rule. All console logging in this test should be migrated to logger.info/log equivalents to maintain consistent instrumentation.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| const nextNewline = this._text.indexOf("\n", this._cursor); | ||
| const lineEnd = nextNewline === -1 ? this._text.length : nextNewline; | ||
|
|
||
| if (lineEnd === this._cursor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleteLineForward() returns without removing the newline when the cursor is at end-of-line, so ctrl+k/meta+Delete stop working there.
Prompt for AI agents
Address the following comment on extensions/cli/src/ui/TextBuffer.ts at line 165:
<comment>deleteLineForward() returns without removing the newline when the cursor is at end-of-line, so ctrl+k/meta+Delete stop working there.</comment>
<file context>
@@ -137,6 +137,38 @@ export class TextBuffer {
+ const nextNewline = this._text.indexOf("\n", this._cursor);
+ const lineEnd = nextNewline === -1 ? this._text.length : nextNewline;
+
+ if (lineEnd === this._cursor) {
+ return;
+ }
</file context>
| // Filter out existing anthropic models | ||
| config.models = config.models.filter( | ||
| (model: any) => !model || model.uses !== "anthropic/claude-4-sonnet", | ||
| (model: any) => !model || model.uses !== "anthropic/claude-sonnet-4-5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By switching the filter to only exclude "anthropic/claude-sonnet-4-5", existing configs that still contain "anthropic/claude-4-sonnet" will keep that stale entry and then add the new model, so the migration fails to swap out the old slug and leaves duplicates.
Prompt for AI agents
Address the following comment on extensions/cli/src/util/yamlConfigUpdater.ts at line 65:
<comment>By switching the filter to only exclude "anthropic/claude-sonnet-4-5", existing configs that still contain "anthropic/claude-4-sonnet" will keep that stale entry and then add the new model, so the migration fails to swap out the old slug and leaves duplicates.</comment>
<file context>
@@ -62,7 +62,7 @@ export function updateAnthropicModelInYaml(
// Filter out existing anthropic models
config.models = config.models.filter(
- (model: any) => !model || model.uses !== "anthropic/claude-4-sonnet",
+ (model: any) => !model || model.uses !== "anthropic/claude-sonnet-4-5",
);
</file context>
| (model: any) => !model || model.uses !== "anthropic/claude-sonnet-4-5", | |
| (model: any) => !model || (model.uses !== "anthropic/claude-sonnet-4-5" && model.uses !== "anthropic/claude-4-sonnet"), |
| updateModelName(null); | ||
| expect(getPersistedModelName()).toBeNull(); | ||
|
|
||
| process.env.CONTINUE_API_KEY = "test-api-key"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating CONTINUE_API_KEY in this test overwrites any existing value and the final delete removes it instead of restoring the original. If the variable was preset before the suite, this leaves later tests without the expected key. Please capture the prior value and restore it after the assertions.
Prompt for AI agents
Address the following comment on extensions/cli/src/integration/model-persistence-unauthenticated.test.ts at line 209:
<comment>Updating CONTINUE_API_KEY in this test overwrites any existing value and the final delete removes it instead of restoring the original. If the variable was preset before the suite, this leaves later tests without the expected key. Please capture the prior value and restore it after the assertions.</comment>
<file context>
@@ -0,0 +1,236 @@
+ updateModelName(null);
+ expect(getPersistedModelName()).toBeNull();
+
+ process.env.CONTINUE_API_KEY = "test-api-key";
+
+ updateModelName("Claude 3.5 Sonnet");
</file context>
✅ Addressed in ad14fcc
| let service = new ModelService(); | ||
| let state = await service.initialize(mockAssistant, mockAuthConfig); | ||
|
|
||
| console.log("Initial model:", state.model?.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule violated: Don't use console.log
New console.log statements were added to this test, but the guideline requires using the structured logger instead of console.* calls. Please swap these console logs out for the approved logging utility (and apply the same change to the other new console.log lines mentioned in the evidence).
Prompt for AI agents
Address the following comment on extensions/cli/src/integration/model-persistence-e2e.test.ts at line 103:
<comment>New console.log statements were added to this test, but the guideline requires using the structured logger instead of console.* calls. Please swap these console logs out for the approved logging utility (and apply the same change to the other new console.log lines mentioned in the evidence).</comment>
<file context>
@@ -0,0 +1,205 @@
+ let service = new ModelService();
+ let state = await service.initialize(mockAssistant, mockAuthConfig);
+
+ console.log("Initial model:", state.model?.name);
+ expect(state.model?.name).toBe("GPT-4");
+
</file context>
| }); | ||
|
|
||
| test("should persist model choice exactly as user experiences it", async () => { | ||
| console.log("\n=== SESSION 1: User opens cn and switches model ==="); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule violated: Don't use console.log
Please replace these new console.log statements with the approved logger API to comply with the "Don't use console.log" rule. All console logging in this test should be migrated to logger.info/log equivalents to maintain consistent instrumentation.
Prompt for AI agents
Address the following comment on extensions/cli/src/integration/model-persistence-user-flow.test.ts at line 95:
<comment>Please replace these new console.log statements with the approved logger API to comply with the "Don't use console.log" rule. All console logging in this test should be migrated to logger.info/log equivalents to maintain consistent instrumentation.</comment>
<file context>
@@ -0,0 +1,154 @@
+ });
+
+ test("should persist model choice exactly as user experiences it", async () => {
+ console.log("\n=== SESSION 1: User opens cn and switches model ===");
+
+ // Session 1: User starts CLI
</file context>
|
🎉 This PR is included in version 1.24.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.21.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
model persistence, cmd+backspace, and a few other minor things that bothered me
Summary by cubic
Persisted CLI model selection across sessions for both logged-in and logged-out users, and refreshed the TUI immediately after a model change. Also scoped line-deletion shortcuts to the current line and updated the default Anthropic model slug.
New Features
Bug Fixes