Skip to content

Commit 90b57e3

Browse files
committed
Fix autocomplete showing mapping keys for empty values
Follow-up to #265 When completing an empty value (e.g., `permissions: |`), mapping keys were incorrectly shown alongside scalar values. This made completions confusing. Before: - `permissions: |` showed read-all, write-all, AND actions, contents, etc. - `on: |` showed check_run AND check_run (full syntax), etc. After: - `permissions: |` shows only read-all and write-all - `on: |` shows only event names like push, check_run - `concurrency: |` shows no completions (user types their own group name) Users who want the mapping form choose (full syntax) completions at the parent level.
1 parent a06ceee commit 90b57e3

File tree

2 files changed

+45
-29
lines changed

2 files changed

+45
-29
lines changed

languageservice/src/complete.test.ts

Lines changed: 34 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -212,28 +212,31 @@ jobs:
212212
expect(result[0].label).toEqual("my-custom-label");
213213
});
214214

215-
it("does not show parent mapping sibling keys", async () => {
215+
it("does not show mapping keys or parent sibling keys in Key mode", async () => {
216+
// At `container: |`, the scalar form is a string with no constants.
217+
// Mapping keys should NOT be shown - users should use `container (full syntax)`.
216218
const input = `on: push
217219
jobs:
218220
build:
219221
container: |
220222
runs-on: ubuntu-latest`;
221223
const result = await complete(...getPositionFromCursor(input));
222224
expect(result).not.toBeUndefined();
223-
expect(result.length).toEqual(6);
224-
// Should not contain other top-level job keys like `if` and `runs-on`
225-
expect(result.map(x => x.label)).not.toContain("if");
226-
expect(result.map(x => x.label)).not.toContain("runs-on");
225+
// No completions because: scalar has no constants, mapping variant skipped in Key mode
226+
expect(result.length).toEqual(0);
227227
});
228228

229-
it("shows mapping keys within a new map ", async () => {
229+
it("does not show mapping keys in Key mode when structure is uncommitted", async () => {
230+
// At `concurrency: |`, user is in Key mode but hasn't committed to a structure.
231+
// The scalar form is a string with no constants, so no completions.
232+
// Mapping keys are NOT shown - users should use `concurrency (full syntax)` at parent level.
230233
const input = `on: push
231234
jobs:
232235
build:
233236
concurrency: |`;
234237
const result = await complete(...getPositionFromCursor(input));
235238
expect(result).not.toBeUndefined();
236-
expect(result.map(x => x.label).sort()).toEqual(["cancel-in-progress", "group"]);
239+
expect(result.map(x => x.label)).toEqual([]);
237240
});
238241

239242
it("job key", async () => {
@@ -475,15 +478,15 @@ jobs:
475478
});
476479
});
477480

478-
it("adds a new line and indentation for mapping keys when the key is given", async () => {
481+
it("does not show mapping keys in Key mode for one-of with mapping variant", async () => {
482+
// At `concurrency: |`, mapping keys should NOT be shown.
483+
// Users who want the mapping form should use `concurrency (full syntax)` at parent level.
479484
const input = "concurrency: |";
480485

481486
const result = await complete(...getPositionFromCursor(input));
482487

483-
expect(result.filter(x => x.label === "cancel-in-progress").map(x => x.textEdit?.newText)).toEqual([
484-
"\n cancel-in-progress: "
485-
]);
486-
expect(result.filter(x => x.label === "group").map(x => x.textEdit?.newText)).toEqual(["\n group: "]);
488+
expect(result.filter(x => x.label === "cancel-in-progress")).toEqual([]);
489+
expect(result.filter(x => x.label === "group")).toEqual([]);
487490
});
488491

489492
it("does not add new line if no key in line", async () => {
@@ -525,8 +528,10 @@ jobs:
525528
expect(result.filter(x => x.label === "types")).toEqual([]);
526529
});
527530

528-
it("shows all options for one-of when user hasn't committed to a type yet", async () => {
529-
// At `permissions: |` user hasn't typed anything yet - show all options
531+
it("shows only scalar options for one-of in Key mode when user hasn't committed to a type", async () => {
532+
// At `permissions: |` user hasn't typed anything yet - show only scalar options
533+
// Mapping keys are NOT shown because they would require a newline
534+
// Users who want the mapping form can use `permissions (full syntax)` at the parent level
530535
const input = "on: push\npermissions: |";
531536

532537
const result = await complete(...getPositionFromCursor(input));
@@ -535,9 +540,9 @@ jobs:
535540
expect(result.filter(x => x.label === "read-all").map(x => x.textEdit?.newText)).toEqual(["read-all"]);
536541
expect(result.filter(x => x.label === "write-all").map(x => x.textEdit?.newText)).toEqual(["write-all"]);
537542

538-
// Mapping keys should also be available (user hasn't committed yet)
539-
expect(result.filter(x => x.label === "actions").map(x => x.textEdit?.newText)).toEqual(["\n actions: "]);
540-
expect(result.filter(x => x.label === "contents").map(x => x.textEdit?.newText)).toEqual(["\n contents: "]);
543+
// Mapping keys should NOT be shown - they require a newline which is confusing inline
544+
expect(result.filter(x => x.label === "actions")).toEqual([]);
545+
expect(result.filter(x => x.label === "contents")).toEqual([]);
541546
});
542547

543548
it("filters to scalar options when user has started typing a scalar", async () => {
@@ -603,17 +608,18 @@ jobs:
603608
expect(result.find(x => x.label === "runs-on (full syntax)")?.textEdit?.newText).toEqual("runs-on:\n ");
604609
});
605610

606-
it("generates correct insertText for one-of variants in key mode", async () => {
607-
// concurrency is a one-of: [string, mapping] - testing key mode (after colon on same line)
608-
const input = "concurrency: |";
611+
it("generates correct insertText for one-of variants in parent mode", async () => {
612+
// concurrency is a one-of: [string, mapping] - testing parent mode (inside mapping)
613+
// At `concurrency:\n |`, user HAS committed to mapping structure, so mapping keys are shown
614+
const input = "concurrency:\n |";
609615

610616
const result = await complete(...getPositionFromCursor(input));
611617

612-
// Scalar in key mode: newline + indented key + colon + space
613-
expect(result.find(x => x.label === "group")?.textEdit?.newText).toEqual("\n group: ");
618+
// In parent mode: just key + colon + space (no leading newline)
619+
expect(result.find(x => x.label === "group")?.textEdit?.newText).toEqual("group: ");
614620

615-
// Boolean in key mode (cancel-in-progress): newline + indented key + colon + space
616-
expect(result.find(x => x.label === "cancel-in-progress")?.textEdit?.newText).toEqual("\n cancel-in-progress: ");
621+
// Boolean in parent mode (cancel-in-progress): key + colon + space
622+
expect(result.find(x => x.label === "cancel-in-progress")?.textEdit?.newText).toEqual("cancel-in-progress: ");
617623
});
618624

619625
it("uses base key as filterText for qualified one-of variants", async () => {
@@ -647,11 +653,10 @@ jobs:
647653
const checkRun = result.find(x => x.label === "check_run");
648654
expect(checkRun?.textEdit?.newText).toEqual("check_run");
649655

650-
// Full syntax form inserts as a mapping key (with newline in Key mode)
651-
// This is expected behavior - it starts the mapping form
652-
const checkRunFull = result.find(x => x.label === "check_run (full syntax)");
653-
// In Key mode: \n + indent + key + : + \n + indent + indent (for nested content)
654-
expect(checkRunFull?.textEdit?.newText).toEqual("\n check_run:\n ");
656+
// Full syntax form should NOT be shown in Key mode - it requires a newline
657+
// which is confusing when typing inline. Users who want the mapping form
658+
// can use `on (full syntax)` at the parent level.
659+
expect(result.find(x => x.label === "check_run (full syntax)")).toBeUndefined();
655660
});
656661

657662
it("filters to sequence options when user has started a sequence", async () => {

languageservice/src/value-providers/definition.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,17 @@ function oneOfValues(
198198
}
199199
}
200200

201+
// In Key mode (after colon, e.g., `on: |`), skip mapping variants when completing
202+
// an empty value. Mapping keys require a newline which is confusing when typing
203+
// inline. Users who want the mapping form can use the `(full syntax)` completion
204+
// at the parent level.
205+
if (!tokenStructure && mode === DefinitionValueMode.Key) {
206+
const variantBucket = getStructuralBucket(variantDef.definitionType);
207+
if (variantBucket === "mapping") {
208+
continue;
209+
}
210+
}
211+
201212
values.push(...definitionValues(variantDef, indentation, mode, tokenStructure));
202213
}
203214
return distinctValues(values);

0 commit comments

Comments
 (0)