fix completions not displayed for field access #970
Conversation
WalkthroughThe changes introduce asynchronous completion handling in the expression editor. Changes
Sequence DiagramsequenceDiagram
participant User
participant Editor as ChipExpressionEditor
participant Waiter as waitForStateChange
participant Source as buildCompletionSource
participant Fetcher as getCompletions
User->>Editor: Types character
Editor->>Editor: Set completionsFetchScheduledRef=true
Editor->>Waiter: Poll for completions
rect rgb(200, 220, 255)
Note over Waiter: Polling phase
loop requestAnimationFrame
Waiter->>Editor: Check completionsFetchScheduledRef
end
end
par Async fetch
Source->>Fetcher: await getCompletions()
Fetcher-->>Source: Promise<CompletionItem[]>
end
Editor->>Editor: Update completionsRef
Editor->>Editor: Set completionsFetchScheduledRef=false
Waiter-->>Source: Return ready completions
rect rgb(220, 240, 220)
Note over Source: Process phase
Source->>Source: Filter & map completions
Source-->>Editor: CompletionResult | null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (1)
400-434: Guard againstwordbeing null when triggering completions after.
context.matchBefore(/\w*/)can returnnull(for example when the last non-space char is.but it's not preceded by a word, such as" ."or".."). In that case, the current guard:if (lastNonSpaceChar !== '.' && ( !word || (word.from === word.to && !context.explicit) )) { return null; }will not return early (because
lastNonSpaceChar === '.'), and the later use ofword.text/word.fromwill throw at runtime.Add a dedicated null check after the special-case
.handling so you never dereferencewordwhen it is absent:- const word = context.matchBefore(/\w*/); - if (lastNonSpaceChar !== '.' && ( - !word || (word.from === word.to && !context.explicit) - )) { - return null; - } + const word = context.matchBefore(/\w*/); + if (lastNonSpaceChar !== '.' && ( + !word || (word.from === word.to && !context.explicit) + )) { + return null; + } + + // Even when triggered by `.`, bail out if no word was found before the cursor. + if (!word) { + return null; + }This preserves the desired “always allow completions after
.when there is a preceding word” behavior, while avoiding a potential runtime error.
🧹 Nitpick comments (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (1)
417-433: Add error handling around asyncgetCompletionsto avoid breaking the completion source
getCompletionsis now async and may rely on RPC or network calls. If it rejects, the exception will propagate into CodeMirror’s completion pipeline, potentially disabling completions or spamming the console.Wrap the await in a
try/catchand fail gracefully:- const completions = await getCompletions(); + let completions: CompletionItem[]; + try { + completions = await getCompletions(); + } catch (error) { + // Swallow failures and let CodeMirror continue operating + console.error("Failed to fetch completions", error); + return null; + }This makes the completion source robust to transient backend failures while keeping the rest of the editor responsive.
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
129-145: ClarifycompletionSourcememoization and its dependency onprops.completions
completionSourceis memoized with:const completionSource = useMemo(() => { return buildCompletionSource(waitForStateChange); }, [props.completions]);But the
EditorViewand itsautocompletion({ override: [completionSource] })extension are created only once in theuseEffectwith an empty dependency array (lines 219–267). This means:
- CodeMirror will always use the first
completionSourceinstance wired into the initialEditorState.- Subsequent re-renders and new
completionSourceinstances (whenprops.completionschanges) are not propagated to the existingEditorView.Because
completionSourceinternally closes overcompletionsRefandcompletionsFetchScheduledRef, which are mutable refs, you don’t actually need to recreate the function whenprops.completionschanges—the single instance will see updated data via the refs.To reduce confusion and avoid unnecessary allocations, you can simplify:
- const completionSource = useMemo(() => { - return buildCompletionSource(waitForStateChange); - }, [props.completions]); + const completionSource = useMemo( + () => buildCompletionSource(waitForStateChange), + [] // stable for the life of this component + );This better reflects how the EditorView is configured in practice and keeps the dependency story simpler.
Also applies to: 219-267
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts(1 hunks)workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx (1)
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/CodeUtils.ts (1)
buildCompletionSource(400-435)
| const completionsRef = useRef<CompletionItem[]>(props.completions); | ||
| const helperPaneToggleButtonRef = useRef<HTMLButtonElement>(null); | ||
| const completionsFetchScheduledRef = useRef<boolean>(false); | ||
|
|
There was a problem hiding this comment.
Avoid unbounded requestAnimationFrame polling when completions don’t arrive
The handshake between completionsFetchScheduledRef and waitForStateChange is generally good, but there’s a failure mode:
- On every doc change,
handleChangeListnersetscompletionsFetchScheduledRef.current = true. waitForStateChangethen recursively polls viarequestAnimationFrameuntilcompletionsFetchScheduledRef.currentbecomesfalse.- It is set back to
falseonly in theuseEffectthat mirrorsprops.completionsintocompletionsRef(line 303–305).
If, for any reason, props.completions isn’t updated (backend error, RPC being disabled, or a parent component choosing not to update on certain edits), completionsFetchScheduledRef.current will stay true and:
waitForStateChange’s Promise will never resolve, leaving CodeMirror’s completion request hanging.requestAnimationFrame(checkState)will run indefinitely every frame, which is a subtle performance leak.
Consider adding a timeout/fallback and ensuring the scheduled flag is always cleared, even on failure:
- const waitForStateChange = (): Promise<CompletionItem[]> => {
- return new Promise((resolve) => {
- const checkState = () => {
- if (!completionsFetchScheduledRef.current) {
- resolve(completionsRef.current);
- } else {
- requestAnimationFrame(checkState);
- }
- };
- checkState();
- });
- };
+ const waitForStateChange = (timeoutMs = 500): Promise<CompletionItem[]> => {
+ const start = performance.now();
+ return new Promise((resolve) => {
+ const checkState = () => {
+ const timedOut = performance.now() - start > timeoutMs;
+ if (!completionsFetchScheduledRef.current || timedOut) {
+ // On timeout, fall back to whatever completions we currently have.
+ completionsFetchScheduledRef.current = false;
+ resolve(completionsRef.current);
+ return;
+ }
+ requestAnimationFrame(checkState);
+ };
+ checkState();
+ });
+ };Additionally, make sure any completion-fetching code that can fail still drives props.completions (or explicitly flips completionsFetchScheduledRef.current back to false in its error path) so the editor doesn’t get stuck waiting.
Also applies to: 102-115, 129-145, 300-305
🤖 Prompt for AI Agents
In
workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/ChipExpressionEditor/components/ChipExpressionEditor.tsx
around lines 91-94 (and similarly 102-115, 129-145, 300-305), the
requestAnimationFrame polling driven by completionsFetchScheduledRef can spin
forever if props.completions never updates; add a bounded timeout/fallback
inside waitForStateChange so the Promise resolves after a configurable max wait
(e.g., 500-1000ms) and ensure completionsFetchScheduledRef.current is cleared on
timeout or any error path; also update any completion-fetching error handlers to
flip completionsFetchScheduledRef.current = false so the editor never remains
stuck waiting.
2 participants
Purpose
The completion suggestions were not appearing when typing certain identifiers such as
user.to access fields of a record type.This occurred because CodeMirror was using stale completion data, as the newly fetched completions were not being synced with the internal
completionSource.Resolves: wso2/product-ballerina-integrator#1841
Goals
user.).Approach
completionSource.waitForStateChange()utility that waits until the updated completion values are applied before CodeMirror handles completion queries.useRefandrequestAnimationFrame, ensuring the editor receives up-to-date data.Summary by CodeRabbit