feat: add dynamic intellisense for parentId/id values in YAML editor #72
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds dynamic intellisense support for parentId and id values in the YAML editor by fetching libraries and collections from the Jellyfin API and providing auto-completion suggestions.
- Adds functionality to fetch libraries and collections from Jellyfin API for auto-completion
- Implements Monaco Editor completion provider for parentId/id fields in YAML
- Integrates the completion provider with proper cleanup on component disposal
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@kamilkosek if you handle copilots comments ill merge |
Co-authored-by: Copilot <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-authored-by: Copilot <[email protected]>
thanks for reminding me :D Was on my todo list - but I am currently stuck in another project so streamyfin stuff gets a bit neglected. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Jellyfin.Plugin.Streamyfin/Pages/YamlEditor/index.js (4)
58-77: Improve suggestion matching and orderingEnhance UX by letting users match by name or id and keep libraries before collections.
- const libSuggestions = libraries + const libSuggestions = libraries .filter(i => i?.Id && i?.Name) .map(i => ({ label: `${i.Name} (${i.Id})`, kind: monaco.languages.CompletionItemKind.Value, insertText: i.Id, + filterText: `${i.Name} ${i.Id}`, + sortText: `1-${i.Name}`, detail: 'Library folder', documentation: i.Path ? `Path: ${i.Path}` : undefined })); @@ - const colSuggestions = collections + const colSuggestions = collections .filter(i => i?.Id && i?.Name) .map(i => ({ label: `${i.Name} (${i.Id})`, kind: monaco.languages.CompletionItemKind.Value, insertText: i.Id, + filterText: `${i.Name} ${i.Id}`, + sortText: `2-${i.Name}`, detail: 'Collection', documentation: i.Overview || undefined }));
96-99: ‘id’ is very generic — consider scoping to avoid over-triggeringMatching any key named
idcan surface suggestions in unrelated contexts. Prefer limiting toparentIdor gatingidby surrounding context (e.g., list items under known sections), or makeidsupport optional/toggled.
105-123: Expand replacement range to include the whole current value (avoid leftovers/extra quotes)Currently only replaces up to the cursor, which can leave trailing characters or closing quotes. Compute the end column to cover the full current value (respecting quotes) before replacing.
- // Compute replacement range: from word start to cursor - const word = model.getWordUntilPosition(position); - const startColFromColon = (() => { - const idx = beforeCursor.lastIndexOf(':'); - if (idx === -1) return word.startColumn; - let start = idx + 1; // first char after colon - // skip spaces - while (start < beforeCursor.length && beforeCursor.charAt(start) === ' ') start++; - // skip optional opening quotes - while (start < beforeCursor.length && (beforeCursor.charAt(start) === '"' || beforeCursor.charAt(start) === "'")) start++; - // Monaco columns are 1-based - return start + 1; - })(); - const range = new monaco.Range( - position.lineNumber, - Math.max(1, startColFromColon), - position.lineNumber, - position.column - ); + // Compute replacement range: from value start to end-of-value + const word = model.getWordUntilPosition(position); + const startInfo = (() => { + const idx = beforeCursor.lastIndexOf(':'); + if (idx === -1) return { startOffset: position.column - 1, startColumn: word.startColumn }; + let start = idx + 1; // first char after colon (0-based) + while (start < beforeCursor.length && beforeCursor.charAt(start) === ' ') start++; // spaces + const hadQuote = beforeCursor.charAt(start) === '"' || beforeCursor.charAt(start) === "'"; + if (hadQuote) start++; // skip opening quote + return { startOffset: start, startColumn: start + 1, hadQuote }; + })(); + const afterCursor = line.substring(position.column - 1); + const endColumn = (() => { + let end = position.column - 1; // 0-based + const quoteChar = startInfo.hadQuote + ? beforeCursor.charAt(startInfo.startOffset - 1) + : null; + if (quoteChar) { + for (let i = 0; i < afterCursor.length; i++) { + const ch = afterCursor.charAt(i); + if (ch === quoteChar || ch === '#' || ch === '\r' || ch === '\n') { + if (ch === quoteChar) end++; // include closing quote + break; + } + end++; + } + } else { + for (let i = 0; i < afterCursor.length; i++) { + const ch = afterCursor.charAt(i); + if (/\s|["'#:,]/.test(ch)) break; + end++; + } + } + return end + 1; // to 1-based + })(); + const range = new monaco.Range( + position.lineNumber, + Math.max(1, startInfo.startColumn), + position.lineNumber, + Math.max(position.column, endColumn) + );
220-224: Also reset cached suggestions on hide to avoid stale data and free memoryResetting lets you refetch after user/library changes in the same app session.
Page?.parentIdProvider?.dispose?.() Page.editor = undefined; Page.yaml = undefined; Page.parentIdProvider = undefined; + Page.parentIdSuggestions = undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Jellyfin.Plugin.Streamyfin/Pages/YamlEditor/index.js(3 hunks)
🔇 Additional comments (1)
Jellyfin.Plugin.Streamyfin/Pages/YamlEditor/index.js (1)
191-193: Nice integration of the provider into init and lifecycleRegistering in
initand disposing onviewhidekeeps things clean.
| const userId = await window.ApiClient.getCurrentUserId?.() ?? null; | ||
|
|
||
| // Build URLs using ApiClient to preserve base path and auth | ||
| // Prefer user views over raw media folders for broader compatibility | ||
| const libsUrl = userId | ||
| ? window.ApiClient.getUrl(`Users/${userId}/Views`) | ||
| : window.ApiClient.getUrl('Library/MediaFolders'); |
There was a problem hiding this comment.
Make await ?? precedence explicit and allow retries after load failure
- Parenthesize
await … ?? nullto avoid precedence gotchas. - On fetch failure you set
parentIdSuggestions = []. That prevents future retries in the same session because the provider checks only for Array-ness. Useundefinedto re-attempt later.
- const userId = await window.ApiClient.getCurrentUserId?.() ?? null;
+ const userId = (await window.ApiClient.getCurrentUserId?.()) ?? null;
@@
- console.warn('Failed to load parentId suggestions', e);
- Page.parentIdSuggestions = [];
+ console.warn('Failed to load parentId suggestions', e);
+ // unset to allow retry on next invocation
+ Page.parentIdSuggestions = undefined;Also applies to: 80-83
🤖 Prompt for AI Agents
In Jellyfin.Plugin.Streamyfin/Pages/YamlEditor/index.js around lines 27-33 (and
also apply the same change at lines 80-83), make the await/nullish-coalescing
precedence explicit by parenthesizing the awaited call so the nullish coalescing
applies to the awaited result (i.e., wrap the await expression in parentheses
before applying ?? null), and when a fetch fails do not set parentIdSuggestions
to an empty array (which prevents retries) but set it to undefined so the
provider can attempt loading again later; apply both changes in the two
mentioned locations.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| console.log("Hiding") | ||
| Page?.editor?.dispose() | ||
| Page?.yaml?.dispose() | ||
| Page?.parentIdProvider?.dispose?.() |
There was a problem hiding this comment.
The optional chaining on dispose suggests uncertainty about the method's existence. Consider checking if dispose is a function before calling it: if (typeof Page?.parentIdProvider?.dispose === 'function') Page.parentIdProvider.dispose()
| Page?.parentIdProvider?.dispose?.() | |
| if (typeof Page?.parentIdProvider?.dispose === 'function') Page.parentIdProvider.dispose(); |
2 participants
Summary by CodeRabbit