Feast manage key bindings in vscode devspace#747
Conversation
msivasubramaniaan
requested review from
RomanNikitenko,
azatsarynnyy,
rgrunber and
vitaliy-guliy
as code owners
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
📝 WalkthroughWalkthroughThis PR adds a new "che-keybindings" VS Code extension that synchronizes user keybindings with a Che backend. It includes the extension manifest, README, webpack/tsconfig setup, a REST API client (GET/PATCH), local keybindings file read/write helpers, a client-ID utility, a Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Extension
participant SyncManager
participant KeybindingsFile
participant CheAPI
Extension->>SyncManager: initialize()
SyncManager->>CheAPI: getKeybindings(namespace)
CheAPI-->>SyncManager: remote keybindings
SyncManager->>KeybindingsFile: writeUserKeybindings(remoteJson)
KeybindingsFile->>SyncManager: onDidSaveTextDocument
SyncManager->>CheAPI: patchKeybindings(namespace, json, clientId)
loop every 10s
SyncManager->>CheAPI: getKeybindings(namespace)
CheAPI-->>SyncManager: remote keybindings
SyncManager->>KeybindingsFile: writeUserKeybindings(if changed)
end
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (3)
code/extensions/che-keybindings/extension.webpack.config.js (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
bufferutil/utf-8-validateexternals likely unnecessary.These are optional native deps of
ws, but this extension's only runtime dependency isnode-fetch. If copied from another extension's webpack config, consider removing since they add no value here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/extensions/che-keybindings/extension.webpack.config.js` around lines 24 - 27, The webpack externals in extension.webpack.config.js include bufferutil and utf-8-validate, but this extension does not use ws and only depends on node-fetch at runtime. Remove the unnecessary externals entries from the config so the extension only declares what it actually needs, and keep the webpack configuration aligned with the extension’s real dependency set.code/extensions/che-keybindings/package.json (1)
15-17: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
activationEvents: ["*"]causes eager activation on every VS Code startup.VS Code's own guidance is to use this activation event in your extension only when no other activation events combination works in your use-case. Given this extension only needs to poll/sync keybindings after startup,
onStartupFinishedwould avoid slowing down initial load.⚡ Suggested fix
"activationEvents": [ - "*" + "onStartupFinished" ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/extensions/che-keybindings/package.json` around lines 15 - 17, The che-keybindings extension is using eager activation via activationEvents with "*", which makes it load on every VS Code startup. Update the package.json activationEvents for this extension to use a startup-complete trigger such as onStartupFinished instead, so activation is deferred until after initial startup. Keep the change localized to the che-keybindings package manifest and ensure the extension still activates when its polling/sync logic is needed.code/extensions/che-keybindings/src/utils.ts (1)
14-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a stronger ID generator and handle
state.update().
Math.random()gives no collision guarantees for a value used to dedupe sync loops (seeSyncManager.pull()comparingremote.meta?.clientId === this.clientId). Alsostate.update()returns aThenablethat's silently ignored here.♻️ Proposed fix
+import { randomUUID } from "node:crypto"; + export function getOrCreateClientId( state: vscode.Memento, output?: vscode.OutputChannel, -): string { +): Promise<string> | string { output?.appendLine("[Utils] Retrieving client ID..."); let id = state.get<string>("clientId"); if (!id) { output?.appendLine( "[Utils] No existing client ID found. Generating a new one...", ); - id = Math.random().toString(36).slice(2); + id = randomUUID(); - state.update("clientId", id); + return Promise.resolve(state.update("clientId", id)).then(() => { + output?.appendLine(`[Utils] Generated new client ID: ${id}`); + return id as string; + }); - - output?.appendLine(`[Utils] Generated new client ID: ${id}`); } else { output?.appendLine(`[Utils] Existing client ID found: ${id}`); } return id; }Note this changes the function signature to async; callers in
syncManager.ts:39would needawait.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/extensions/che-keybindings/src/utils.ts` around lines 14 - 37, `getOrCreateClientId` should stop using `Math.random()` for a dedupe-sensitive client identifier and should handle the asynchronous `state.update()` result. Update the function to use a stronger unique ID generator, make it async so the `vscode.Memento.update` promise is awaited, and adjust callers such as `SyncManager.pull()` to await `getOrCreateClientId` before comparing `remote.meta?.clientId` against `this.clientId`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/extensions/che-keybindings/package.json`:
- Around line 49-58: The contributed commands cheKeybindingsSync.pull and
cheKeybindingsSync.push are declared but not wired up, so add matching
vscode.commands.registerCommand handlers in che-keybindings/src/extension.ts or
che-keybindings/src/syncManager.ts. Use the existing SyncManager flow to
implement the pull and push actions, and make sure the command IDs exactly match
the package.json entries so the Command Palette can invoke them successfully.
- Around line 62-69: Remove the unused cheKeybindingsSync.storageMode
configuration entry from the che-keybindings extension manifest, since it is
never consumed by the extension logic. Update the package.json contribution
settings so the local/global enum and default are no longer exposed to users,
and verify there are no references to cheKeybindingsSync.storageMode elsewhere
in the extension before deleting it.
In `@code/extensions/che-keybindings/src/cheAPi.ts`:
- Around line 25-55: Both keybinding API requests can hang indefinitely, so add
a timeout-driven abort signal to the fetch calls in getKeybindings and
patchKeybindings. Update the request flow in cheAPi.ts to create an
AbortController (or equivalent timeout helper), pass its signal into each fetch,
and ensure the timeout is cleared once the request completes. Keep the existing
logging and return behavior unchanged, but make sure stalled backend calls fail
fast instead of blocking sync or piling up overlapping polls.
- Around line 14-21: The KeybindingsResponse metadata currently carries clientId
only, which filters self-echoes but does not protect against concurrent updates.
Thread resourceVersion through the PATCH flow in the keybindings API and related
handling so updates include and compare the latest version before writing, using
the existing KeybindingsResponse.meta shape and any PATCH handler or client
methods that consume it.
In `@code/extensions/che-keybindings/src/keybindings.ts`:
- Around line 19-41: The readUserKeybindings() fallback to "[]" is masking real
read failures and can cause SyncManager.pull() to overwrite valid local
keybindings; change readUserKeybindings() in che-keybindings/src/keybindings.ts
to fail loudly by propagating the exception instead of returning an empty array
string, while keeping the existing logging in the catch path so
SyncManager.pull() can hit its outer try/catch and abort the sync cycle safely.
In `@code/extensions/che-keybindings/src/syncManager.ts`:
- Around line 46-70: The polling loop in initialize() can start a new pull()
before the previous one finishes, which can interleave with onSave() and corrupt
shared sync state like suppressPush and lastSynced. Add an overlap guard in
SyncManager so only one pull runs at a time, and have the timer skip or queue if
a pull is already in progress. Use the existing pull() and onSave() paths as the
coordination points, and keep the guard around the setInterval callback and any
direct pull invocations.
---
Nitpick comments:
In `@code/extensions/che-keybindings/extension.webpack.config.js`:
- Around line 24-27: The webpack externals in extension.webpack.config.js
include bufferutil and utf-8-validate, but this extension does not use ws and
only depends on node-fetch at runtime. Remove the unnecessary externals entries
from the config so the extension only declares what it actually needs, and keep
the webpack configuration aligned with the extension’s real dependency set.
In `@code/extensions/che-keybindings/package.json`:
- Around line 15-17: The che-keybindings extension is using eager activation via
activationEvents with "*", which makes it load on every VS Code startup. Update
the package.json activationEvents for this extension to use a startup-complete
trigger such as onStartupFinished instead, so activation is deferred until after
initial startup. Keep the change localized to the che-keybindings package
manifest and ensure the extension still activates when its polling/sync logic is
needed.
In `@code/extensions/che-keybindings/src/utils.ts`:
- Around line 14-37: `getOrCreateClientId` should stop using `Math.random()` for
a dedupe-sensitive client identifier and should handle the asynchronous
`state.update()` result. Update the function to use a stronger unique ID
generator, make it async so the `vscode.Memento.update` promise is awaited, and
adjust callers such as `SyncManager.pull()` to await `getOrCreateClientId`
before comparing `remote.meta?.clientId` against `this.clientId`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07fc250d-4501-4915-ab29-4021fca3453f
⛔ Files ignored due to path filters (2)
code/extensions/che-keybindings/images/eclipse-che-logo.pngis excluded by!**/*.pngcode/extensions/che-keybindings/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.github/workflows/pull-request-check-licenses.ymlcode/build/gulpfile.extensions.tscode/build/npm/dirs.tscode/extensions/che-keybindings/.vscodeignorecode/extensions/che-keybindings/README.mdcode/extensions/che-keybindings/extension.webpack.config.jscode/extensions/che-keybindings/package.jsoncode/extensions/che-keybindings/package.nls.jsoncode/extensions/che-keybindings/src/cheAPi.tscode/extensions/che-keybindings/src/extension.tscode/extensions/che-keybindings/src/keybindings.tscode/extensions/che-keybindings/src/syncManager.tscode/extensions/che-keybindings/src/utils.tscode/extensions/che-keybindings/tsconfig.json
| "commands": [ | ||
| { | ||
| "command": "cheKeybindingsSync.pull", | ||
| "title": "Keybindings: Pull from Che" | ||
| }, | ||
| { | ||
| "command": "cheKeybindingsSync.push", | ||
| "title": "Keybindings: Push to Che" | ||
| } | ||
| ], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "registerCommand" code/extensions/che-keybindings/srcRepository: che-incubator/che-code
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json =="
sed -n '1,220p' code/extensions/che-keybindings/package.json
echo
echo "== source files =="
fd -a . code/extensions/che-keybindings/src
echo
echo "== search for command ids and registrations =="
rg -n "cheKeybindingsSync\.(pull|push)|registerCommand|commands\s*:" code/extensions/che-keybindings
echo
echo "== likely activation and sync manager files =="
for f in \
code/extensions/che-keybindings/src/extension.ts \
code/extensions/che-keybindings/src/syncManager.ts \
code/extensions/che-keybindings/src/sync-manager.ts
do
[ -f "$f" ] && { echo "--- $f ---"; cat -n "$f"; }
doneRepository: che-incubator/che-code
Length of output: 10643
Register the contributed keybinding sync commands
cheKeybindingsSync.pull and cheKeybindingsSync.push are declared in code/extensions/che-keybindings/package.json, but there’s no corresponding vscode.commands.registerCommand in code/extensions/che-keybindings/src/extension.ts or code/extensions/che-keybindings/src/syncManager.ts. Invoking them from the Command Palette will fail unless handlers are added.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/extensions/che-keybindings/package.json` around lines 49 - 58, The
contributed commands cheKeybindingsSync.pull and cheKeybindingsSync.push are
declared but not wired up, so add matching vscode.commands.registerCommand
handlers in che-keybindings/src/extension.ts or
che-keybindings/src/syncManager.ts. Use the existing SyncManager flow to
implement the pull and push actions, and make sure the command IDs exactly match
the package.json entries so the Command Palette can invoke them successfully.
| "cheKeybindingsSync.storageMode": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "local", | ||
| "global" | ||
| ], | ||
| "default": "global" | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "storageMode" code/extensions/che-keybindings/srcRepository: che-incubator/che-code
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files code/extensions/che-keybindings | sed -n '1,200p'
printf '\n== package.json excerpt ==\n'
sed -n '1,220p' code/extensions/che-keybindings/package.json
printf '\n== src search for related config keys ==\n'
rg -n "cheKeybindingsSync|namespace|storageMode|keybindings\.json|vscode-userdata" code/extensions/che-keybindings -S
printf '\n== source outlines ==\n'
for f in $(git ls-files 'code/extensions/che-keybindings/src/*' 2>/dev/null); do
echo "--- $f"
wc -l "$f"
doneRepository: che-incubator/che-code
Length of output: 5797
Remove the unused cheKeybindingsSync.storageMode setting. The extension never reads this config, so the local/global option has no effect and will confuse users.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/extensions/che-keybindings/package.json` around lines 62 - 69, Remove
the unused cheKeybindingsSync.storageMode configuration entry from the
che-keybindings extension manifest, since it is never consumed by the extension
logic. Update the package.json contribution settings so the local/global enum
and default are no longer exposed to users, and verify there are no references
to cheKeybindingsSync.storageMode elsewhere in the extension before deleting it.
| export interface KeybindingsResponse { | ||
| keybindingsJson: string; | ||
|
|
||
| meta?: { | ||
| clientId?: string; | ||
| resourceVersion?: string; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding code paths.
git ls-files code/extensions/che-keybindings/src/cheAPi.ts
wc -l code/extensions/che-keybindings/src/cheAPi.ts
cat -n code/extensions/che-keybindings/src/cheAPi.ts | sed -n '1,220p'
# Find all uses of resourceVersion / clientId in this extension.
rg -n "resourceVersion|clientId|patchKeybindings|getKeybindings" code/extensions/che-keybindings -SRepository: che-incubator/che-code
Length of output: 4834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the caller to understand whether conflict detection is handled elsewhere.
cat -n code/extensions/che-keybindings/src/syncManager.ts | sed -n '1,220p'
# Search for any backend or protocol references to resourceVersion / optimistic concurrency.
rg -n "resourceVersion|If-Match|etag|conflict|409|clientId" -S .Repository: che-incubator/che-code
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the sync flow and any local handling of server metadata.
cat -n code/extensions/che-keybindings/src/syncManager.ts | sed -n '1,220p'
cat -n code/extensions/che-keybindings/src/utils.ts | sed -n '1,160p'
# Search only within the che-keybindings extension for protocol fields and conflict handling.
rg -n "resourceVersion|If-Match|etag|conflict|409|meta\\." code/extensions/che-keybindings/src -SRepository: che-incubator/che-code
Length of output: 7234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the backend API and any schema that mentions keybindingsJson/resourceVersion.
rg -n "keybindingsJson|resourceVersion|clientId" code -SRepository: che-incubator/che-code
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant sync manager logic and any local API/schema references.
sed -n '1,220p' code/extensions/che-keybindings/src/syncManager.ts | cat -n
sed -n '1,140p' code/extensions/che-keybindings/src/utils.ts | cat -n
# Search narrowly for backend or protocol handling of keybinding metadata.
rg -n "/keybindings|resourceVersion|clientId|If-Match|409" code/extensions/che-keybindings code/src -S --glob '!**/node_modules/**'Repository: che-incubator/che-code
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the backend implementation or docs for the keybindings API only.
rg -n "/namespace/.*/keybindings|keybindingsJson|resourceVersion" code/extensions code/src -S --glob '!**/node_modules/**' --max-count 200Repository: che-incubator/che-code
Length of output: 2036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow to the che-keybindings extension files to see whether resourceVersion is propagated anywhere else.
rg -n "resourceVersion|patchKeybindings|getKeybindings|keybindingsJson|clientId" code/extensions/che-keybindings/src -S --max-count 80Repository: che-incubator/che-code
Length of output: 2847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any docs or tests that describe the keybindings sync payload/semantics.
fd -a -t f -g '*keybindings*' code/extensions/che-keybindings code | sed -n '1,200p'
rg -n "che-keybindings|keybindings sync|resourceVersion|clientId" code/extensions/che-keybindings -S --glob '!**/node_modules/**'Repository: che-incubator/che-code
Length of output: 4071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the built-in user data sync model for how resourceVersion is treated.
rg -n "resourceVersion" code/src/vs/platform/userDataSync code/src/vs/workbench/services/userDataProfile -S --max-count 80Repository: che-incubator/che-code
Length of output: 160
Thread resourceVersion through PATCH to prevent stale overwrites. clientId only suppresses self-echoes on pull; it does not detect concurrent edits. A second client can still overwrite newer keybindings without any conflict signal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/extensions/che-keybindings/src/cheAPi.ts` around lines 14 - 21, The
KeybindingsResponse metadata currently carries clientId only, which filters
self-echoes but does not protect against concurrent updates. Thread
resourceVersion through the PATCH flow in the keybindings API and related
handling so updates include and compare the latest version before writing, using
the existing KeybindingsResponse.meta shape and any PATCH handler or client
methods that consume it.
| export async function getKeybindings( | ||
| namespace: string, | ||
| output?: vscode.OutputChannel, | ||
| ): Promise<KeybindingsResponse | null> { | ||
| output?.appendLine(`[API] GET ${API}/namespace/${namespace}/keybindings`); | ||
|
|
||
| const res = await fetch(`${API}/namespace/${namespace}/keybindings`, { | ||
| credentials: "include", | ||
| }); | ||
|
|
||
| output?.appendLine(`[API] GET Status: ${res.status}`); | ||
|
|
||
| if (!res.ok) { | ||
| output?.appendLine(`[API] GET Failed: ${await res.text()}`); | ||
| return null; | ||
| } | ||
|
|
||
| const data = (await res.json()) as KeybindingsResponse; | ||
|
|
||
| output?.appendLine( | ||
| `[API] GET Success - Received ${data.keybindingsJson.length} bytes`, | ||
| ); | ||
|
|
||
| if (data.meta) { | ||
| output?.appendLine( | ||
| `[API] Meta => clientId=${data.meta.clientId ?? "N/A"}, resourceVersion=${data.meta.resourceVersion ?? "N/A"}`, | ||
| ); | ||
| } | ||
|
|
||
| return data; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '(^|/)code/extensions/che-keybindings/src/(cheAPi|syncManager|.*types.*|.*api.*)\.(ts|tsx|js|jsx)$|(^|/)code/extensions/che-keybindings/src/'
printf '\nOutline cheAPi.ts:\n'
ast-grep outline code/extensions/che-keybindings/src/cheAPi.ts --view expanded || true
printf '\nOutline syncManager.ts:\n'
ast-grep outline code/extensions/che-keybindings/src/syncManager.ts --view expanded || true
printf '\nRelevant type/search hits:\n'
rg -n "resourceVersion|AbortSignal\.timeout|fetch\(" code/extensions/che-keybindings/src -SRepository: che-incubator/che-code
Length of output: 1629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cheAPi.ts (1-140) ---'
cat -n code/extensions/che-keybindings/src/cheAPi.ts | sed -n '1,140p'
echo
echo '--- syncManager.ts (1-220) ---'
cat -n code/extensions/che-keybindings/src/syncManager.ts | sed -n '1,220p'Repository: che-incubator/che-code
Length of output: 8701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search for resourceVersion and keybindings API server:\n'
rg -n "resourceVersion|namespace/.*/keybindings|keybindingsJson|clientId" . -S
printf '\nPotential server-side files:\n'
git ls-files | rg '(^|/)(server|backend|api|routes|handlers|controllers|services|src)/.*\.(ts|js|tsx|jsx)$' | head -n 200Repository: che-incubator/che-code
Length of output: 50378
Add a timeout to both keybinding API calls. getKeybindings() and patchKeybindings() can hang forever on a stalled backend, which blocks sync work and can stack up overlapping poll requests. Use an abort signal/timeout on both fetch() calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/extensions/che-keybindings/src/cheAPi.ts` around lines 25 - 55, Both
keybinding API requests can hang indefinitely, so add a timeout-driven abort
signal to the fetch calls in getKeybindings and patchKeybindings. Update the
request flow in cheAPi.ts to create an AbortController (or equivalent timeout
helper), pass its signal into each fetch, and ensure the timeout is cleared once
the request completes. Keep the existing logging and return behavior unchanged,
but make sure stalled backend calls fail fast instead of blocking sync or piling
up overlapping polls.
| export async function readUserKeybindings( | ||
| output?: vscode.OutputChannel, | ||
| ): Promise<string> { | ||
| output?.appendLine("[Keybindings] Reading user keybindings..."); | ||
|
|
||
| try { | ||
| const doc = await vscode.workspace.openTextDocument(USER_KEYBINDINGS_URI); | ||
|
|
||
| output?.appendLine( | ||
| `[Keybindings] Opened: ${USER_KEYBINDINGS_URI.toString()}`, | ||
| ); | ||
|
|
||
| const text = doc.getText(); | ||
|
|
||
| output?.appendLine(`[Keybindings] Read successful (${text.length} bytes).`); | ||
|
|
||
| return text; | ||
| } catch (err) { | ||
| output?.appendLine(`[Keybindings] Failed to read keybindings: ${err}`); | ||
|
|
||
| return "[]"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Swallowing read failures as "[]" risks overwriting real local keybindings.
pull() (syncManager.ts lines 141-157) uses this return value to decide whether to overwrite the local file with remote data. If the actual read fails transiently while the local file has real content, this fallback makes pull() think local is empty/different and unconditionally overwrites it with remote content, silently discarding un-synced local changes.
🛡️ Proposed fix
try {
const doc = await vscode.workspace.openTextDocument(USER_KEYBINDINGS_URI);
...
return text;
} catch (err) {
output?.appendLine(`[Keybindings] Failed to read keybindings: ${err}`);
-
- return "[]";
+ throw err;
}Then in SyncManager.pull(), the existing outer try/catch (syncManager.ts lines 122-164) will log "Pull failed" and skip the cycle instead of proceeding with a masked empty state.
📝 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.
| export async function readUserKeybindings( | |
| output?: vscode.OutputChannel, | |
| ): Promise<string> { | |
| output?.appendLine("[Keybindings] Reading user keybindings..."); | |
| try { | |
| const doc = await vscode.workspace.openTextDocument(USER_KEYBINDINGS_URI); | |
| output?.appendLine( | |
| `[Keybindings] Opened: ${USER_KEYBINDINGS_URI.toString()}`, | |
| ); | |
| const text = doc.getText(); | |
| output?.appendLine(`[Keybindings] Read successful (${text.length} bytes).`); | |
| return text; | |
| } catch (err) { | |
| output?.appendLine(`[Keybindings] Failed to read keybindings: ${err}`); | |
| return "[]"; | |
| } | |
| } | |
| export async function readUserKeybindings( | |
| output?: vscode.OutputChannel, | |
| ): Promise<string> { | |
| output?.appendLine("[Keybindings] Reading user keybindings..."); | |
| try { | |
| const doc = await vscode.workspace.openTextDocument(USER_KEYBINDINGS_URI); | |
| output?.appendLine( | |
| `[Keybindings] Opened: ${USER_KEYBINDINGS_URI.toString()}`, | |
| ); | |
| const text = doc.getText(); | |
| output?.appendLine(`[Keybindings] Read successful (${text.length} bytes).`); | |
| return text; | |
| } catch (err) { | |
| output?.appendLine(`[Keybindings] Failed to read keybindings: ${err}`); | |
| throw err; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/extensions/che-keybindings/src/keybindings.ts` around lines 19 - 41, The
readUserKeybindings() fallback to "[]" is masking real read failures and can
cause SyncManager.pull() to overwrite valid local keybindings; change
readUserKeybindings() in che-keybindings/src/keybindings.ts to fail loudly by
propagating the exception instead of returning an empty array string, while
keeping the existing logging in the catch path so SyncManager.pull() can hit its
outer try/catch and abort the sync cycle safely.
| async initialize() { | ||
| this.output.appendLine("Initializing SyncManager..."); | ||
|
|
||
| try { | ||
| await this.pull(); | ||
| this.output.appendLine("Initial pull completed."); | ||
| } catch (err) { | ||
| this.output.appendLine(`Initial pull failed: ${err}`); | ||
| } | ||
|
|
||
| this.context.subscriptions.push( | ||
| vscode.workspace.onDidSaveTextDocument(this.onSave, this), | ||
| ); | ||
|
|
||
| this.output.appendLine("Registered onDidSaveTextDocument listener."); | ||
|
|
||
| this.timer = setInterval(() => { | ||
| this.output.appendLine("Polling backend for remote keybindings..."); | ||
| this.pull().catch((err) => | ||
| this.output.appendLine(`Polling failed: ${err}`), | ||
| ); | ||
| }, 10000); | ||
|
|
||
| this.output.appendLine("Polling started (every 10 seconds)."); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Overlapping polls can race on shared sync state.
setInterval fires every 10s without waiting for the previous pull() (lines 119-165) to finish. Since pull() and onSave() share mutable state (suppressPush, lastSynced), concurrent invocations can interleave: one pull's suppressPush = true / write / save sequence can be corrupted by another concurrent pull, risking spurious pushes back to the backend or lastSynced being set from stale data.
🔒 Proposed fix (guard against overlap)
+ private pulling = false;
+
async initialize() {
...
- this.timer = setInterval(() => {
+ const scheduleNext = () => {
+ this.timer = setTimeout(() => {
+ this.output.appendLine("Polling backend for remote keybindings...");
+ this.pull()
+ .catch((err) => this.output.appendLine(`Polling failed: ${err}`))
+ .finally(scheduleNext);
+ }, 10000);
+ };
+ scheduleNext();
- this.output.appendLine("Polling backend for remote keybindings...");
- this.pull().catch((err) =>
- this.output.appendLine(`Polling failed: ${err}`),
- );
- }, 10000);
}
dispose() {
...
if (this.timer) {
- clearInterval(this.timer);
+ clearTimeout(this.timer);
}
}
async pull() {
+ if (this.pulling) {
+ this.output.appendLine("Pull already in progress. Skipping.");
+ return;
+ }
+ this.pulling = true;
try {
...
+ } finally {
+ this.pulling = false;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/extensions/che-keybindings/src/syncManager.ts` around lines 46 - 70, The
polling loop in initialize() can start a new pull() before the previous one
finishes, which can interleave with onSave() and corrupt shared sync state like
suppressPush and lastSynced. Add an overlap guard in SyncManager so only one
pull runs at a time, and have the timer skip or queue if a pull is already in
progress. Use the existing pull() and onSave() paths as the coordination points,
and keep the guard around the setInterval callback and any direct pull
invocations.
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
msivasubramaniaan
changed the title
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-747-amd64 |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-747-amd64 |
2 participants
What does this PR do?
This PR will manage key bindings in devspace
What issues does this PR fix?
https://redhat.atlassian.net/browse/CRW-9166
How to test this PR?
Does this PR contain changes that override default upstream Code-OSS behavior?
git rebasewere added to the .rebase folderSummary by CodeRabbit
New Features
Bug Fixes
Documentation