feat: docs tab content #13661
feat: docs tab content #13661
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds DocumentationInfo and DocumentationBaseInfo API types, a DocumentationService that fetches/parses docs.json and tutorials.json (with fallback to a bundled JSON), unit tests for the service, and a fallback-documentation asset. Wires the service into the main plugin DI and IPC handlers ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 |
|
So far this is just a try to one of many approaches to getting content into searchbar docs tab |
37e30b1 to
e2eb0c8
Compare
1bc8308 to
92255f6
Compare
92255f6 to
8da2b0d
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The CommandPalette.svelte component is getting quite large and handles multiple modes now—consider extracting the documentation-tab logic and action execution into smaller subcomponents or utility modules for better readability and testability.
- Using regex to parse HTML in DocumentationService.parseDocumentationContent can be brittle—consider using a proper HTML parser (e.g., DOMParser) to reliably extract links and metadata.
- Avoid magic numeric indices for searchOptionsSelectedIndex by introducing a named enum or constants for each search mode to make the mode-switching logic clearer and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The CommandPalette.svelte component is getting quite large and handles multiple modes now—consider extracting the documentation-tab logic and action execution into smaller subcomponents or utility modules for better readability and testability.
- Using regex to parse HTML in DocumentationService.parseDocumentationContent can be brittle—consider using a proper HTML parser (e.g., DOMParser) to reliably extract links and metadata.
- Avoid magic numeric indices for searchOptionsSelectedIndex by introducing a named enum or constants for each search mode to make the mode-switching logic clearer and less error-prone.
## Individual Comments
### Comment 1
<location> `packages/main/src/plugin/documentation/documentation-service.ts:83` </location>
<code_context>
+ }
+ }
+
+ private parseDocumentationContent(docsHtml: string, tutorialHtml: string): DocumentationInfo[] {
+ const documentation: DocumentationInfo[] = [];
+
</code_context>
<issue_to_address>
Consider replacing manual regex parsing with a DOMParser helper and consolidating core page definitions into a shared constant to simplify the code.
```suggestion
Consider swapping the custom‐regex parsing for a small DOMParser-based helper and collapsing the duplicated “core” pages into a shared constant. This will flatten the nested loops, remove manual regex concerns, and DRY up your core/fallback lists.
1) Add a shared core pages constant and use it in both methods:
```ts
// at top of class
private static readonly CORE_PAGES: DocumentationInfo[] = [
{
id: 'docs-intro',
title: 'Introduction & Getting Started',
description: 'Learn the basics of Podman Desktop',
url: 'https://podman-desktop.io/docs/intro',
category: 'Documentation',
},
{
id: 'tutorial-index',
title: 'Tutorials & Guides',
description: 'Step-by-step tutorials for common tasks',
url: 'https://podman-desktop.io/tutorial',
category: 'Tutorial',
},
];
// replace methods
private getCoreDocumentationPages() {
return DocumentationService.CORE_PAGES;
}
private getFallbackDocumentation() {
return [
...DocumentationService.CORE_PAGES,
{ id: 'docs-containers', /* … */ },
{ id: 'docs-kubernetes', /* … */ },
/* remaining extra fallback pages */
];
}
```
2) Extract link‐parsing into a DOM helper:
```ts
private extractLinks(
html: string,
category: string,
segment: string
): DocumentationInfo[] {
const doc = new DOMParser().parseFromString(html, 'text/html');
return Array.from(doc.querySelectorAll(`a[href*="${segment}"]`))
.map(a => {
const href = a.getAttribute('href')!;
const title = a.textContent?.trim() || '';
const url = href.startsWith('http') ? href : `https://podman-desktop.io${href}`;
return {
id: `${category}-${this.generateId(title)}`,
title,
description: `${category}: ${title}`,
url,
category,
};
})
.filter(({ title }) => title && !/(Edit this page|Next)/.test(title));
}
```
3) Simplify `parseDocumentationContent`:
```ts
private parseDocumentationContent(docsHtml: string, tutorialHtml: string) {
if (!docsHtml || !tutorialHtml) {
console.warn('Missing HTML…');
return this.getCoreDocumentationPages();
}
const docsLinks = this.extractLinks(docsHtml, 'Documentation', '/docs/');
const tutorialLinks = this.extractLinks(tutorialHtml, 'Tutorial', '/tutorial/');
return [
...this.getCoreDocumentationPages(),
...this.removeDuplicates([...docsLinks, ...tutorialLinks]),
];
}
```
This removes regex complexity, flattens loops, and coalesces duplicated page definitions without changing functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| private parseDocumentationContent(docsHtml: string, tutorialHtml: string): DocumentationInfo[] { |
There was a problem hiding this comment.
issue (complexity): Consider replacing manual regex parsing with a DOMParser helper and consolidating core page definitions into a shared constant to simplify the code.
| private parseDocumentationContent(docsHtml: string, tutorialHtml: string): DocumentationInfo[] { | |
| Consider swapping the custom‐regex parsing for a small DOMParser-based helper and collapsing the duplicated “core” pages into a shared constant. This will flatten the nested loops, remove manual regex concerns, and DRY up your core/fallback lists. | |
| 1) Add a shared core pages constant and use it in both methods: | |
| ```ts | |
| // at top of class | |
| private static readonly CORE_PAGES: DocumentationInfo[] = [ | |
| { | |
| id: 'docs-intro', | |
| title: 'Introduction & Getting Started', | |
| description: 'Learn the basics of Podman Desktop', | |
| url: 'https://podman-desktop.io/docs/intro', | |
| category: 'Documentation', | |
| }, | |
| { | |
| id: 'tutorial-index', | |
| title: 'Tutorials & Guides', | |
| description: 'Step-by-step tutorials for common tasks', | |
| url: 'https://podman-desktop.io/tutorial', | |
| category: 'Tutorial', | |
| }, | |
| ]; | |
| // replace methods | |
| private getCoreDocumentationPages() { | |
| return DocumentationService.CORE_PAGES; | |
| } | |
| private getFallbackDocumentation() { | |
| return [ | |
| ...DocumentationService.CORE_PAGES, | |
| { id: 'docs-containers', /* … */ }, | |
| { id: 'docs-kubernetes', /* … */ }, | |
| /* remaining extra fallback pages */ | |
| ]; | |
| } |
- Extract link‐parsing into a DOM helper:
private extractLinks(
html: string,
category: string,
segment: string
): DocumentationInfo[] {
const doc = new DOMParser().parseFromString(html, 'text/html');
return Array.from(doc.querySelectorAll(`a[href*="${segment}"]`))
.map(a => {
const href = a.getAttribute('href')!;
const title = a.textContent?.trim() || '';
const url = href.startsWith('http') ? href : `https://podman-desktop.io${ href}`;
return {
id: `${category}-${this.generateId(title)}`,
title,
description: `${category}: ${title}`,
url,
category,
};
})
.filter(({ title }) => title && !/(Edit this page|Next)/.test(title));
}- Simplify
parseDocumentationContent:
private parseDocumentationContent(docsHtml: string, tutorialHtml: string) {
if (!docsHtml || !tutorialHtml) {
console.warn('Missing HTML…');
return this.getCoreDocumentationPages();
}
const docsLinks = this.extractLinks(docsHtml, 'Documentation', '/docs/');
const tutorialLinks = this.extractLinks(tutorialHtml, 'Tutorial', '/tutorial/');
return [
...this.getCoreDocumentationPages(),
...this.removeDuplicates([...docsLinks, ...tutorialLinks]),
];
}This removes regex complexity, flattens loops, and coalesces duplicated page definitions without changing functionality.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (1)
199-211: Prevent potential crash when scrollElements entry is missingSwitch to optional chaining; filtered list length can change between renders, leaving holes.
- scrollElements[selectedFilteredIndex].scrollIntoView({ + scrollElements[selectedFilteredIndex]?.scrollIntoView({ @@ - scrollElements[selectedFilteredIndex].scrollIntoView({ + scrollElements[selectedFilteredIndex]?.scrollIntoView({Also applies to: 218-223
🧹 Nitpick comments (14)
packages/api/src/documentation-info.ts (1)
19-25: Tighten the type surface (readonly fields + discriminated category).Make fields immutable and prevent category typos with a literal union.
+export type DocumentationCategory = 'Documentation' | 'Tutorial'; -export interface DocumentationInfo { - id: string; - title: string; - description: string; - url: string; - category: string; -} +export interface DocumentationInfo { + readonly id: string; + readonly title: string; + readonly description: string; + readonly url: string; + readonly category: DocumentationCategory; +}packages/main/src/plugin/documentation/documentation-service.spec.ts (1)
25-27: Restore global fetch after tests to avoid cross-suite leaks.Overriding a global without restoration can create hard-to-debug flakes.
-const mockFetch = vi.fn(); -global.fetch = mockFetch; +const originalFetch = global.fetch; +const mockFetch = vi.fn(); +global.fetch = mockFetch; + +// ... + +afterAll(() => { + global.fetch = originalFetch; +});packages/main/src/plugin/index.ts (1)
2102-2109: IPC wiring for docs get/refresh is solid; consider event consumption guidance.Service likely emits 'documentation-updated' via ApiSender; ensure the renderer subscribes to
window.events.receive('documentation-updated', ...)to refresh UI reactively afterdocumentation:refresh.packages/renderer/src/lib/dialogs/CommandPalette.spec.ts (4)
43-59: Make window stubs configurable to avoid hard-to-debug test pollutionDefine mocked window properties with configurable: true so subsequent tests (or other suites) can redefine/override them cleanly.
- Object.defineProperty(window, 'executeCommand', { - value: executeCommandMock, - }); + Object.defineProperty(window, 'executeCommand', { + value: executeCommandMock, + configurable: true, + }); @@ - Object.defineProperty(window, 'openExternal', { - value: openExternalMock, - }); + Object.defineProperty(window, 'openExternal', { + value: openExternalMock, + configurable: true, + }); @@ - Object.defineProperty(window, 'getOsPlatform', { - value: getOsPlatformMock, - }); + Object.defineProperty(window, 'getOsPlatform', { + value: getOsPlatformMock, + configurable: true, + }); @@ - Object.defineProperty(window, 'getDocumentationItems', { - value: getDocumentationItemsMock, - }); + Object.defineProperty(window, 'getDocumentationItems', { + value: getDocumentationItemsMock, + configurable: true, + });
436-454: Avoid variable-based RegExp in queries; use exact string matchSwitch to a string name to remove the variable-RegExp warning and reduce flakiness.
- const expectedTab = screen.getByRole('button', { name: new RegExp(expectedTabText, 'i') }); + const expectedTab = screen.getByRole('button', { name: expectedTabText });
441-454: Prefer role/aria over CSS-class assertions for tab selectionAsserting styling classes is brittle. If possible, assert semantic state (e.g., role="tab" + aria-selected). See Svelte change below to expose aria-selected; then here query getAllByRole('tab') and assert aria-selected.
If you adopt the Svelte change to add role="tab" and aria-selected, replace these assertions with:
- const allTabButtons = screen.getAllByRole('button').filter( /* class-based filter */ ); + const allTabButtons = screen.getAllByRole('tab'); @@ - expect(allTabButtons[0]).toHaveClass('text-[var(--pd-button-tab-text-selected)]'); + expect(allTabButtons[0]).toHaveAttribute('aria-selected', 'true');Also applies to: 477-485
396-418: Add a test that documentation items open externallyWe’re mocking getDocumentationItems; add a positive-path test to ensure docs are listed and open via window.openExternal.
+ test('Documentation tab shows items and opens selection', async () => { + const docs = [{ id: 'docs-intro', title: 'Intro', description: 'Start', url: 'https://podman-desktop.io/docs/intro', category: 'Documentation' }]; + getDocumentationItemsMock.mockResolvedValueOnce(docs); + + render(CommandPalette); + await userEvent.keyboard('{Control>}k{/Control}'); // open on Documentation tab + const input = screen.getByRole('textbox', { name: COMMAND_PALETTE_ARIA_LABEL }); + expect(input).toBeInTheDocument(); + + // item should appear and be selected by default + const item = await screen.findByRole('button', { name: /Intro/ }); + await userEvent.click(item); + expect(openExternalMock).toHaveBeenCalledWith('https://podman-desktop.io/docs/intro'); + });packages/main/src/plugin/documentation/documentation-service.ts (2)
36-46: Gracefully degrade if only one endpoint succeedsDon’t discard successfully fetched content if the other page fails. Parse what’s available and add core/fallback entries for the missing set.
- const [docsContent, tutorialContent] = await Promise.all([ + const [docsContent, tutorialContent] = await Promise.allSettled([ this.fetchPageContent('https://podman-desktop.io/docs/intro'), - this.fetchPageContent('https://podman-desktop.io/tutorial'), + this.fetchPageContent('https://podman-desktop.io/tutorial'), ]); - - if (docsContent && tutorialContent) { - this.documentation = this.parseDocumentationContent(docsContent, tutorialContent); - } else { - throw new Error('Failed to fetch documentation content'); - } + const docsOk = docsContent.status === 'fulfilled' ? docsContent.value : undefined; + const tutOk = tutorialContent.status === 'fulfilled' ? tutorialContent.value : undefined; + if (docsOk || tutOk) { + this.documentation = this.parseDocumentationContent(docsOk ?? '', tutOk ?? ''); + } else { + throw new Error('Failed to fetch documentation content'); + }
83-131: Regex-on-HTML is fragile; prefer DOM parsingAnchors often contain nested elements and attributes; regex will miss them or misparse. Consider a lightweight HTML parser (e.g., node-html-parser) to select 'a' tags under known containers and extract href/textContent safely, then resolve relative URLs.
I can provide a small parser helper using node-html-parser and targeted selectors if you’d like.
packages/renderer/src/lib/dialogs/CommandPalette.svelte (5)
236-241: Reset selection and refocus when switching tabsKeeps UX consistent and prevents stale indices across modes.
function switchSearchOption(direction: 1 | -1): void { const searchOptionsLength = searchOptions.length; const offset = direction === 1 ? 0 : searchOptionsLength; searchOptionsSelectedIndex = (searchOptionsSelectedIndex + direction + offset) % searchOptionsLength; + selectedFilteredIndex = 0; + tick().then(() => inputElement?.focus()).catch(() => {}); }
339-356: Apply the same reset when clicking a tab buttonMirror keyboard tab-switch logic for mouse interaction.
- on:click={(): void => { - searchOptionsSelectedIndex = index; - }} + on:click={(): void => { + searchOptionsSelectedIndex = index; + selectedFilteredIndex = 0; + tick().then(() => inputElement?.focus()).catch(() => {}); + }}
364-371: Use Svelte’s on:click instead of inline onclickKeeps event binding consistent and avoids confusion with DOM inline handlers.
- <button - onclick={(): Promise<void> => clickOnItem(i)} + <button + on:click={(): Promise<void> => clickOnItem(i)}
282-285: Fix spelling: hideCommandPallete ➜ hideCommandPaletteRename function and call sites.
-function hideCommandPallete(): void { +function hideCommandPalette(): void { display = false; onclose?.(); } @@ - hideCommandPallete(); + hideCommandPalette(); @@ - hideCommandPallete(); + hideCommandPalette(); @@ - hideCommandPallete(); + hideCommandPalette(); @@ - hideCommandPallete(); + hideCommandPalette();Also applies to: 174-176, 225-229, 273-279, 288-294
131-153: Listen for 'documentation-updated' to refresh items reactivelyWhen main triggers a refresh, update the renderer list without reopening the palette.
if (display && documentationItems.length === 0 && !isLoadingDocumentation) { @@ } + // react to updates pushed from main + window?.events?.receive?.('documentation-updated', async () => { + try { + isLoadingDocumentation = true; + documentationItems = await window.getDocumentationItems(); + } catch (e) { + console.error('Failed to refresh documentation items', e); + } finally { + isLoadingDocumentation = false; + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
packages/api/src/documentation-info.ts(1 hunks)packages/main/src/plugin/documentation/documentation-service.spec.ts(1 hunks)packages/main/src/plugin/documentation/documentation-service.ts(1 hunks)packages/main/src/plugin/index.ts(4 hunks)packages/preload/src/index.ts(2 hunks)packages/renderer/src/lib/dialogs/CommandPalette.spec.ts(14 hunks)packages/renderer/src/lib/dialogs/CommandPalette.svelte(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/main/src/plugin/documentation/documentation-service.ts (1)
packages/api/src/documentation-info.ts (1)
DocumentationInfo(19-25)
packages/renderer/src/lib/dialogs/CommandPalette.spec.ts (1)
tests/playwright/src/model/pages/command-palette.ts (1)
CommandPalette(24-44)
packages/preload/src/index.ts (1)
packages/api/src/documentation-info.ts (1)
DocumentationInfo(19-25)
packages/main/src/plugin/index.ts (1)
packages/api/src/documentation-info.ts (1)
DocumentationInfo(19-25)
🪛 ast-grep (0.38.6)
packages/renderer/src/lib/dialogs/CommandPalette.spec.ts
[warning] 436-436: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(expectedTabText, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (8)
packages/api/src/documentation-info.ts (1)
1-17: License header looks good.SPDX and Apache-2.0 header are correct and consistent with the repo.
packages/preload/src/index.ts (2)
65-65: Type-only import is correct.Tree-shake friendly and consistent with the file’s import style.
1602-1609: IPC bridge for docs: API shape is clear and consistent.Channel names match main process handlers; return types are precise. LGTM.
packages/main/src/plugin/documentation/documentation-service.spec.ts (1)
137-173: The scripts were triggered to locate and inspect thefetchDocumentationandgetDocumentationItemsmethods. Once we have their implementations, we can confirm whether the service re-fetches on each call or caches results after initialization.packages/main/src/plugin/index.ts (3)
87-87: Public type import is correct.Keeps runtime bundle clean while providing types to IPC handlers.
162-162: Service import aligns with DI usage below.No concerns.
725-727: DI binding for DocumentationService is appropriate.Singleton scope is the right choice for caching and event emission.
packages/main/src/plugin/documentation/documentation-service.ts (1)
37-39: Tutorials URL path is correct The code already uses/tutorial, which matches the official tutorials index.
| private async fetchPageContent(url: string): Promise<string> { | ||
| try { | ||
| const response = await fetch(url); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP ${response.< 6D40 span class=pl-c1>status}: ${response.statusText}`); | ||
| } | ||
|
|
||
| return await response.text(); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| if (error.name === 'AbortError') { | ||
| throw new Error(`Request timeout while fetching ${url}`); | ||
| } | ||
| throw new Error(`Failed to fetch ${url}: ${error.message}`); | ||
| } | ||
| throw new Error(`Failed to fetch ${url}: Unknown error`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add request timeouts to fetchPageContent (AbortController)
You already handle AbortError but never set a timeout. Add AbortController to avoid hung network calls.
- private async fetchPageContent(url: string): Promise<string> {
+ private async fetchPageContent(url: string): Promise<string> {
try {
- const response = await fetch(url);
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 8000);
+ const response = await fetch(url, {
+ signal: controller.signal,
+ headers: { Accept: 'text/html' },
+ });
+ clearTimeout(timeout);
@@
- } catch (error) {
+ } catch (error) {
if (error instanceof Error) {
if (error.name === 'AbortError') {
throw new Error(`Request timeout while fetching ${url}`);
}
throw new Error(`Failed to fetch ${url}: ${error.message}`);
}
throw new Error(`Failed to fetch ${url}: Unknown error`);
}
}📝 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.
| private async fetchPageContent(url: string): Promise<string> { | |
| try { | |
| const response = await fetch(url); | |
| if (!response.ok) { | |
| throw new Error(`HTTP ${response.status}: ${response.statusText}`); | |
| } | |
| return await response.text(); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| if (error.name === 'AbortError') { | |
| throw new Error(`Request timeout while fetching ${url}`); | |
| } | |
| throw new Error(`Failed to fetch ${url}: ${error.message}`); | |
| } | |
| throw new Error(`Failed to fetch ${url}: Unknown error`); | |
| } | |
| } | |
| private async fetchPageContent(url: string): Promise<string> { | |
| try { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 8000); | |
| const response = await fetch(url, { | |
| signal: controller.signal, | |
| headers: { Accept: 'text/html' }, | |
| }); | |
| clearTimeout(timeout); | |
| if (!response.ok) { | |
| throw new Error(`HTTP ${response.status}: ${response.statusText}`); | |
| } | |
| return await response.text(); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| if (error.name === 'AbortError') { | |
| throw new Error(`Request timeout while fetching ${url}`); | |
| } | |
| throw new Error(`Failed to fetch ${url}: ${error.message}`); | |
| } | |
| throw new Error(`Failed to fetch ${url}: Unknown error`); | |
| } | |
| } |
🤖 Prompt for AI Agents
packages/main/src/plugin/documentation/documentation-service.ts around lines 63
to 81: fetchPageContent currently checks for AbortError but never sets a
timeout, so network requests can hang; add an AbortController, pass its signal
to fetch, start a timer (e.g., const timeoutId = setTimeout(() =>
controller.abort(), <desired ms>)) before calling fetch, clearTimeout(timeoutId)
after a successful response or when throwing, and in the catch keep the
AbortError branch throwing a clear timeout error message; ensure the controller
and timeout are properly typed/cleaned up to avoid leaks.
packages/main/src/plugin/documentation/documentation-service.ts
Outdated
Show resolved
Hide resolved
| onMount(async () => { | ||
| const platform = await window.getOsPlatform(); | ||
| isMac = platform === 'darwin'; | ||
| }); | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard getOsPlatform() with try/catch to avoid unhandled rejections
If the preload API isn’t available, this throws on mount.
-onMount(async () => {
- const platform = await window.getOsPlatform();
- isMac = platform === 'darwin';
-});
+onMount(async () => {
+ try {
+ const platform = await window.getOsPlatform();
+ isMac = platform === 'darwin';
+ } catch (e) {
+ console.warn('getOsPlatform unavailable; defaulting to non-mac', e);
+ isMac = false;
+ }
+});📝 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.
| onMount(async () => { | |
| const platform = await window.getOsPlatform(); | |
| isMac = platform === 'darwin'; | |
| }); | |
| onMount(async () => { | |
| try { | |
| const platform = await window.getOsPlatform(); | |
| isMac = platform === 'darwin'; | |
| } catch (e) { | |
| console.warn('getOsPlatform unavailable; defaulting to non-mac', e); | |
| isMac = false; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/renderer/src/lib/dialogs/CommandPalette.svelte around lines 100 to
104, the onMount call awaits window.getOsPlatform() without guard
6D38
ing against the
preload API being missing which can cause an unhandled rejection; wrap the await
in a try/catch (or first check typeof window.getOsPlatform === 'function') and
set a safe default for isMac (e.g., false) in the catch so mount never throws
and isMac remains defined.
8da2b0d to
2065c1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
packages/api/src/documentation-info.ts (1)
19-25: Consider making the DTO immutable and lightly documented.Readonly fields prevent accidental mutation across renderer/preload/main. Minimal JSDoc improves DX without changing runtime.
Apply:
-export interface DocumentationInfo { - id: string; - title: string; - description: string; - url: string; - category: string; -} +/** + * Canonical documentation entry rendered in the Command Palette "Documentation" tab. + * All fields are intended to be immutable. + */ +export interface DocumentationInfo { + readonly id: string; + readonly title: string; + readonly description: string; + readonly url: string; + readonly category: string; +}packages/renderer/src/lib/dialogs/CommandPalette.spec.ts (9)
21-23: Using fireEvent directly is fine; prefer userEvent.keyboard for key semantics.userEvent synthesizes realistic key sequences (keydown/keypress/keyup) and default modifiers. Not required to change, but it can reduce flakiness.
43-59: Make window property mocks configurable/writable to ease cleanup.defineProperty defaults to non-configurable; future overrides/teardown might fail. Mark them configurable/writable.
Apply:
Object.defineProperty(window, 'executeCommand', { - value: executeCommandMock, + value: executeCommandMock, + configurable: true, + writable: true, }); Object.defineProperty(window, 'openExternal', { - value: openExternalMock, + value: openExternalMock, + configurable: true, + writable: true, }); Object.defineProperty(window, 'getOsPlatform', { - value: getOsPlatformMock, + value: getOsPlatformMock, + configurable: true, + writable: true, }); Object.defineProperty(window, 'getDocumentationItems', { - value: getDocumentationItemsMock, + value: getDocumentationItemsMock, + configurable: true, + writable: true, });
117-129: Navigation test reads well; minor robustness nit.Firing ArrowDown on window matches a global listener. If the component ever scopes handlers, prefer userEvent.keyboard('{ArrowDown}') after focusing the input.
Also applies to: 135-141, 156-157
181-191: Same nit as above for ArrowUp tests.Switching to userEvent.keyboard would keep event ordering realistic.
Also applies to: 198-217, 226-227
246-268: Enter key test is clear; assert side effects only once.Optional: also assert that the palette is closed if that’s expected UX.
298-331: Filtering test is solid; minor stability suggestion.Prefer findByRole for the post-filter first item to avoid any render timing edge cases before submitting Enter.
366-368: Avoid arbitrary sleeps to reduce flakiness.The context is set before render; the 100ms delay is unnecessary.
Apply:
- // wait a little bit for the context to be updated - await new Promise(resolve => setTimeout(resolve, 100));
386-394: Enablement test: Enter triggers first enabled item — good.Optional: also verify disabled items aren’t focusable/selectable via keyboard.
396-455: Tab selection assertions rely on CSS classes; prefer ARIA state.If feasible, expose tabs with role="tab" and aria-selected to assert selection. This improves a11y and makes tests less brittle to style refactors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/api/src/documentation-info.ts(1 hunks)packages/main/src/plugin/documentation/documentation-service.spec.ts(1 hunks)packages/main/src/plugin/documentation/documentation-service.ts(1 hunks)packages/main/src/plugin/index.ts(4 hunks)packages/preload/src/index.ts(2 hunks)packages/renderer/src/lib/dialogs/CommandPalette.spec.ts(13 hunks)packages/renderer/src/lib/dialogs/CommandPalette.svelte(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/preload/src/index.ts
- packages/main/src/plugin/documentation/documentation-service.ts
- packages/main/src/plugin/index.ts
- packages/main/src/plugin/documentation/documentation-service.spec.ts
- packages/renderer/src/lib/dialogs/CommandPalette.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / windows-2025
- GitHub Check: smoke e2e tests (development)
- GitHub Check: smoke e2e tests (production)
- GitHub Check: typecheck
- GitHub Check: linter, formatters
- GitHub Check: Windows
- GitHub Check: build website
- GitHub Check: Linux
- GitHub Check: macOS
🔇 Additional comments (5)
packages/api/src/documentation-info.ts (1)
1-17: License header looks good.Apache-2.0 header is correctly included and formatted.
packages/renderer/src/lib/dialogs/CommandPalette.spec.ts (4)
31-35: Mocks declaration LGTM.Clear separation of mocks improves readability.
61-63: Good defaults for platform and docs items.Keeps tests deterministic across OS.
457-505: Forward Tab cycling test is comprehensive.Good coverage of wrap-around behavior.
507-555: Backward Shift+Tab cycling test is comprehensive.Mirrors forward flow and validates wrap-around.
| export interface DocumentationInfo { | ||
| id: string; | ||
| title: string; | ||
| description: string; | ||
| url: string; | ||
| category: string; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify import path consistency for this type.
Some codebases import TS types with a .js suffix for ESM compatibility; others omit it. Ensure consistent usage to avoid resolution issues in mixed CJS/ESM and when running tests.
Run:
🏁 Script executed:
#!/bin/bash
# Find all imports of documentation-info with/without .js suffix
rg -nP "from ['\"]/@api/documentation-info(\.js)?['\"]" -C2Length of output: 2219
Unify import paths for DocumentationInfo
Imports of DocumentationInfo are inconsistent: in packages/preload/src/index.ts (line 65) and packages/renderer/src/lib/dialogs/CommandPalette.svelte (line 11) the path omits “.js”, but in packages/main/src/plugin/... the path includes “.js”. Update the preload and renderer imports to use /@api/documentation-info.js for ESM compatibility.
🤖 Prompt for AI Agents
In packages/api/src/documentation-info.ts around lines 19–25, the
DocumentationInfo interface is exported but other packages import it with
inconsistent paths; update the imports in packages/preload/src/index.ts (line
~65) and packages/renderer/src/lib/dialogs/CommandPalette.svelte (line ~11) to
use the ESM-compatible path "/@api/documentation-info.js" (replacing any imports
that omit the .js extension), and verify the project alias/resolution
configuration still resolves /@api to the compiled API bundle.
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
…chbar Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz> Assisted by: Cursor
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
e5e43b6 to
c274493
Compare
|
@axel7083 @jiridostal can you PTAL? |
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz> Asisted by: Cursor
c274493 to
cf19386
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/main/src/plugin/documentation/documentation-service.ts (2)
139-142: Deduplicate across core pages and parsed items.The current implementation only removes duplicates within the parsed
documentationarray but does not deduplicate betweencorePagesand parsed items. If the JSON contains entries that match core pages, they will appear twice in the final list.Apply this diff to deduplicate across all items:
// Remove duplicates and add core pages const corePages = this.getCoreDocumentationPages(); - return [...corePages, ...this.removeDuplicates(documentation)]; + return this.removeDuplicates([...corePages, ...documentation]);Based on previous review comments.
62-67: Return a copy and dedupe concurrent initialization calls.
getDocumentationItems()has two issues:
- It returns the internal
documentationarray by reference, allowing callers to mutate internal state.- Concurrent calls before initialization will trigger multiple
fetchDocumentation()calls, causing redundant network requests.Apply this diff to address both concerns:
export class DocumentationService extends Disposable { private documentation: DocumentationInfo[] = []; private isInitialized = false; + private initPromise?: Promise<void>; constructor( @inject(ApiSenderType) private apiSender: ApiSenderType, ) { super(() => { this.documentation = []; this.isInitialized = false; + this.initPromise = undefined; }); } async getDocumentationItems(): Promise<DocumentationInfo[]> { if (!this.isInitialized) { - await this.fetchDocumentation(); + this.initPromise ??= this.fetchDocumentation().finally(() => { + this.initPromise = undefined; + }); + await this.initPromise; } - return this.documentation; + return [...this.documentation]; }Also update
refreshDocumentation()to reset the promise:async refreshDocumentation(): Promise<void> { this.isInitialized = false; // Force re-fetch + this.initPromise = undefined; await this.fetchDocumentation(); this.apiSender.send('documentation-updated'); }Based on previous review comments.
🧹 Nitpick comments (2)
packages/main/src/plugin/documentation/documentation-service.ts (2)
26-39: Consider using @preDestroy() instead of extending Disposable.As suggested in a previous review, using inversify's
@preDestroy()decorator would be more idiomatic for dependency-injected classes. This would eliminate the need to extendDisposableand callsuper()with a cleanup function.Apply this diff to use
@preDestroy():+import { inject, injectable, preDestroy } from 'inversify'; + -import { Disposable } from '../types/disposable.js'; @injectable() -export class DocumentationService extends Disposable { +export class DocumentationService { private documentation: DocumentationInfo[] = []; private isInitialized = false; constructor( @inject(ApiSenderType) private apiSender: ApiSenderType, - ) { - super(() => { - this.documentation = []; - this.isInitialized = false; - }); - } + ) {} + + @preDestroy() + dispose(): void { + this.documentation = []; + this.isInitialized = false; + }Based on previous review comments.
144-208: Consolidate core pages into a shared constant to eliminate duplication.The same two core items (
docs-introandtutorial-index) are duplicated in bothgetCoreDocumentationPages()andgetFallbackDocumentation(). This makes maintenance harder and increases the risk of inconsistency.Apply this diff to use a shared constant:
@injectable() export class DocumentationService extends Disposable { + private static readonly CORE_PAGES: DocumentationInfo[] = [ + { + id: 'docs-intro', + title: 'Introduction & Getting Started', + description: 'Learn the basics of Podman Desktop', + url: 'https://podman-desktop.io/docs/intro', + category: 'Documentation', + }, + { + id: 'tutorial-index', + title: 'Tutorials & Guides', + description: 'Step-by-step tutorials for common tasks', + url: 'https://podman-desktop.io/tutorial', + category: 'Tutorial', + }, + ]; + private documentation: DocumentationInfo[] = []; private isInitialized = false; // ... private getCoreDocumentationPages(): DocumentationInfo[] { - return [ - { - id: 'docs-intro', - title: 'Introduction & Getting Started', - description: 'Learn the basics of Podman Desktop', - url: 'https://podman-desktop.io/docs/intro', - category: 'Documentation', - }, - { - id: 'tutorial-index', - title: 'Tutorials & Guides', - description: 'Step-by-step tutorials for common tasks', - url: 'https://podman-desktop.io/tutorial', - category: 'Tutorial', - }, - ]; + return DocumentationService.CORE_PAGES; } private getFallbackDocumentation(): DocumentationInfo[] { return [ - { - id: 'docs-intro', - title: 'Introduction & Getting Started', - description: 'Learn the basics of Podman Desktop', - url: 'https://podman-desktop.io/docs/intro', - category: 'Documentation', - }, - { - id: 'tutorial-index', - title: 'Tutorials & Guides', - description: 'Step-by-step tutorials for common tasks', - url: 'https://podman-desktop.io/tutorial', - category: 'Tutorial', - }, + ...DocumentationService.CORE_PAGES, { id: 'docs-containers', title: 'Containers Documentation', description: 'Working with containers, images, and pods', url: 'https://podman-desktop.io/docs/containers', category: 'Documentation', }, // ... rest of fallback items ]; }Based on previous review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/main/src/plugin/documentation/documentation-service.spec.ts(1 hunks)packages/main/src/plugin/documentation/documentation-service.ts(1 hunks)packages/preload/src/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/preload/src/index.ts
- packages/main/src/plugin/documentation/documentation-service.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/main/src/plugin/documentation/documentation-service.ts (2)
packages/extension-api/src/extension-api.d.ts (1)
Disposable(90-118)packages/api/src/documentation-info.ts (2)
DocumentationInfo(24-30)DocumentationJsonInfo(19-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke e2e tests (development)
- GitHub Check: smoke e2e tests (production)
- GitHub Check: unit tests / windows-2025
- GitHub Check: Windows
- GitHub Check: linter, formatters
- GitHub Check: typecheck
- GitHub Check: build website
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: Sourcery review
🔇 Additional comments (2)
packages/main/src/plugin/documentation/documentation-service.ts (2)
210-246: LGTM: Utility methods are well-implemented.Both
removeDuplicates()andgenerateId()are correctly implemented with appropriate defensive programming:
removeDuplicates()efficiently uses a Set for O(1) lookups and validates items before processing.generateId()handles edge cases (invalid input, empty strings, trailing dashes) and generates safe, URL-friendly IDs with a reasonable length limit.
69-73: apiSender.send invocation is correct
Thesendmethod’sdataparameter is optional (data?), so omitting the payload is valid.
| async fetchDocumentation(): Promise<void> { | ||
| try { | ||
| const [docsJson, tutorialsJson] = await Promise.all([ | ||
| this.fetchJsonContent('https://podman-desktop.io/docs.json'), | ||
| this.fetchJsonContent('https://podman-desktop.io/tutorials.json'), | ||
| ]); | ||
|
|
||
| if (docsJson && tutorialsJson) { | ||
| this.documentation = this.parseDocumentationFromJson(docsJson, tutorialsJson); | ||
| } else { | ||
| throw new Error('Failed to fetch documentation JSON files'); | ||
| } | ||
| this.isInitialized = true; | ||
| } catch (error) { | ||
| console.error('Failed to fetch documentation at startup:', error); | ||
| // Fallback to predefined documentation if fetching fails | ||
| this.documentation = this.getFallbackDocumentation(); | ||
| this.isInitialized = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add request timeout to prevent hanging network calls.
The fetch operations in fetchDocumentation() do not have a timeout, which could cause the method to hang indefinitely if the network is slow or unresponsive. While the fallback mechanism handles errors, it won't help if the request simply hangs.
Consider adding a ti
65CE
meout using AbortController:
async fetchDocumentation(): Promise<void> {
try {
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 8000);
+
const [docsJson, tutorialsJson] = await Promise.all([
- this.fetchJsonContent('https://podman-desktop.io/docs.json'),
- this.fetchJsonContent('https://podman-desktop.io/tutorials.json'),
+ this.fetchJsonContent('https://podman-desktop.io/docs.json', controller.signal),
+ this.fetchJsonContent('https://podman-desktop.io/tutorials.json', controller.signal),
]);
+ clearTimeout(timeoutId);
if (docsJson && tutorialsJson) {
this.documentation = this.parseDocumentationFromJson(docsJson, tutorialsJson);
} else {
throw new Error('Failed to fetch documentation JSON files');
}
this.isInitialized = true;
} catch (error) {
console.error('Failed to fetch documentation at startup:', error);
// Fallback to predefined documentation if fetching fails
this.documentation = this.getFallbackDocumentation();
this.isInitialized = true;
}
}Update fetchJsonContent signature to accept an optional signal:
-private async fetchJsonContent(url: string): Promise<Array<{ name: string; url: string }> | null> {
+private async fetchJsonContent(url: string, signal?: AbortSignal): Promise<Array<{ name: string; url: string }> | null> {
try {
- const response = await fetch(url);
+ const response = await fetch(url, { signal });Based on previous review comments.
📝 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.
| async fetchDocumentation(): Promise<void> { | |
| try { | |
| const [docsJson, tutorialsJson] = await Promise.all([ | |
| this.fetchJsonContent('https://podman-desktop.io/docs.json'), | |
| this.fetchJsonContent('https://podman-desktop.io/tutorials.json'), | |
| ]); | |
| if (docsJson && tutorialsJson) { | |
| this.documentation = this.parseDocumentationFromJson(docsJson, tutorialsJson); | |
| } else { | |
| throw new Error('Failed to fetch documentation JSON files'); | |
| } | |
| this.isInitialized = true; | |
| } catch (error) { | |
| console.error('Failed to fetch documentation at startup:', error); | |
| // Fallback to predefined documentation if fetching fails | |
| this.documentation = this.getFallbackDocumentation(); | |
| this.isInitialized = true; | |
| } | |
| } | |
| // packages/main/src/plugin/documentation/documentation-service.ts | |
| // Updated fetchDocumentation with timeout | |
| async fetchDocumentation(): Promise<void> { | |
| try { | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 8000); | |
| const [docsJson, tutorialsJson] = await Promise.all([ | |
| this.fetchJsonContent('https://podman-desktop.io/docs.json', controller.signal), | |
| this.fetchJsonContent('https://podman-desktop.io/tutorials.json', controller.signal), | |
| ]); | |
| clearTimeout(timeoutId); | |
| if (docsJson && tutorialsJson) { | |
| this.documentation = this.parseDocumentationFromJson(docsJson, tutorialsJson); | |
| } else { | |
| throw new Error('Failed to fetch documentation JSON files'); | |
| } | |
| this.isInitialized = true; | |
| } catch (error) { | |
| console.error('Failed to fetch d 38BA ocumentation at startup:', error); | |
| // Fallback to predefined documentation if fetching fails | |
| this.documentation = this.getFallbackDocumentation(); | |
| this.isInitialized = true; | |
| } | |
| } | |
| // Also update fetchJsonContent to accept an optional AbortSignal: | |
| private async fetchJsonContent( | |
| url: string, | |
| signal?: AbortSignal | |
| ): Promise<Array<{ name: string; url: string }> | null> { | |
| try { | |
| const response = await fetch(url, { signal }); | |
| if (!response.ok) { | |
| console.warn(`Failed to fetch JSON ${url}: ${response.statusText}`); | |
| return null; | |
| } | |
| return await response.json(); | |
| } catch (error: any) { | |
| if (error.name === 'AbortError') { | |
| console.warn(`Request timeout while fetching ${url}`); | |
| return null; | |
| } | |
| console.error(`Error fetching JSON ${url}:`, error); | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/main/src/plugin/documentation/documentation-service.ts around lines
41 to 60, the fetchDocumentation method performs parallel fetches without a
timeout which can hang; update fetchJsonContent to accept an optional
AbortSignal parameter and use AbortController here to enforce a timeout (create
a controller, set a setTimeout to call controller.abort() after a reasonable
delay, pass controller.signal into each fetchJ
D306
sonContent call, and clear the
timeout on success), ensure fetchJsonContent propagates AbortError so the catch
block falls back to getFallbackDocumentation, and handle clearing/cleanup of the
timer to avoid leaks.
| async getDocumentationItems(): Promise<DocumentationInfo[]> { | ||
| if (!this.isInitialized) { | ||
| await this.fetchDocumentation(); | ||
| } | ||
| return this.documentation; | ||
| } |
| throw new Error('Failed to fetch documentation JSON files'); | ||
| } | ||
| this.isInitialized = true; | ||
| } catch (error) { |
There was a problem hiding this comment.
| } catch (error) { | |
| } catch (error: unkown) { |
no type error here?
| this.apiSender.send('documentation-updated'); | ||
| } | ||
|
|
||
| private async fetchJsonContent(url: string): Promise<Array<{ name: string; url: string }> | null> { |
There was a problem hiding this comment.
we do not use null, eslint is happy about that??
There was a problem hiding this comment.
let's avoid returning null, either return something valid or throw an error, let's avoid the caller to handler yet another error scenario
| throw new Error(`HTTP ${response.status}: ${response.statusText}`); | ||
| } | ||
|
|
||
| return await response.json(); |
There was a problem hiding this comment.
Basic validation? At least it ensure the payload is an array
| docsJson: DocumentationJsonInfo[], | ||
| tutorialsJson: DocumentationJsonInfo[], |
There was a problem hiding this comment.
why using the wording json in interfaces name?
| return [...corePages, ...this.removeDuplicates(documentation)]; | ||
| } | ||
|
|
||
| private getCoreDocumentationPages(): DocumentationInfo[] { |
There was a problem hiding this comment.
Please move this to a dedicated asset file somewhere, I would rather not have hardcoded documention info in the typescript files
| ]; | ||
| } | ||
|
|
||
| private getFallbackDocumentation(): DocumentationInfo[] { |
There was a problem hiding this comment.
| ]; | ||
| } | ||
|
|
||
| private removeDuplicates(items: DocumentationInfo[]): DocumentationInfo[] { |
There was a problem hiding this comment.
Why is this needed ? I would rather have the docusaurus script ensuring that all links are unique
| }); | ||
| } | ||
|
|
||
| private generateId(title: string): string { |
There was a problem hiding this comment.
is the id gonna be visible to the user ? otherwise just hash the title
| {#if searchOptionsSelectedIndex === 2} | ||
| <Button icon={faChevronRight} type='link' onclick={(): Promise<void> => window.openExternal('https://podman-desktop.io/docs')}>Browse All Documentation</Button> | ||
| {:else} | ||
| <Button icon={faChevronRight} type='link' onclick={(): void => {console.log('Variables clicked');}}>Variables</Button> |
There was a problem hiding this comment.
Not sure what is the purpose of this "Variables" with console log?
There was a problem hiding this comment.
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)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (1)
346-367: Align type discrimination with executeAction.Line 347 uses the same unreliable
'category' in itemcheck. For consistency and correctness, use the same discriminator as inexecuteAction.Apply this diff:
- {#each filteredItems as item, i (i)} - {@const isDocItem = 'category' in item} + {#each filteredItems as item, i (i)} + {@const isDocItem = 'url' in item && typeof (item as any).url === 'string'} <li class="flex w-full flex-row" bind:this={scrollElements[i]} aria-label={item.id}>
♻️ Duplicate comments (3)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (3)
74-83: Fix null-safe filtering; optional chaining is incomplete.The optional chaining stops at
toLowerCase(), so.includes()is called onundefinedwhen fields are missing, causing a runtime exception.Apply this diff:
let filteredDocumentationInfoItems: DocumentationInfo[] = $derived( documentationItems.filter(item => inputValue - ? item.name?.toLowerCase().includes(inputValue.toLowerCase()) || - item.description?.toLowerCase().includes(inputValue.toLowerCase()) || - item.category?.toLowerCase().includes(inputValue.toLowerCase()) + ? item.name?.toLowerCase()?.includes(inputValue.toLowerCase()) || + item.description?.toLowerCase()?.includes(inputValue.toLowerCase()) || + item.category?.toLowerCase()?.includes(inputValue.toLowerCase()) : true, ), );
227-254: Fix type discrimination and guard IPC calls.Using
'category' in itemis unreliable becauseCommandInfomay also have acategoryproperty. Additionally, the IPC bridge functions lack existence checks.Apply this diff:
async function executeAction(index: number): Promise<void> { const item = filteredItems[index]; if (!item) return; - // Check if it's a documentation item by checking for 'category' property - const isDocItem = 'category' in item; + // Discriminate by presence of url field (docs) vs id field (commands) + const isDocItem = 'url' in item && typeof (item as any).url === 'string'; if (isDocItem) { // Documentation item const docItem = item as DocumentationInfo; - if (docItem.url) { - try { - await window.openExternal(docItem.url); - } catch (error) { - console.error('Error opening documentation URL', error); - } - } + if (!docItem.url) return; + try { + const u = new URL(docItem.url); + if (u.protocol === 'http:' || u.protocol === 'https:') { + if (typeof window.openExternal === 'function') { + await window.openExternal(u.toString()); + } else { + console.warn('openExternal unavailable'); + } + } else { + console.warn('Blocked non-http(s) URL:', docItem.url); + } + } catch (error) { + console.error('Invalid documentation URL', error); + } } else { // Command item const commandItem = item as CommandInfo; - if (commandItem.id) { - try { - await window.executeCommand(commandItem.id); - } catch (error) { - console.error('error executing command', error); - } - } + if (!commandItem.id) return; + try { + if (typeof window.executeCommand === 'function') { + await window.executeCommand(commandItem.id); + } else { + console.warn('executeCommand unavailable'); + } + } catch (error) { + console.error('error executing command', error); + } } }
378-378: Remove or implement the Variables button.The
onclickhandler only logs to the console, indicating incomplete implementation. Either implement the intended functionality or remove this placeholder.Consider removing the button until the "Go to" mode functionality is implemented:
- {:else} - <Button icon={faChevronRight} type='link' onclick={(): void => {console.log('Variables clicked');}}>Variables</Button> {/if}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/api/src/documentation-info.ts(1 hunks)packages/main/src/assets/fallback-documentation.json(1 hunks)packages/main/src/plugin/documentation/documentation-service.spec.ts(1 hunks)packages/main/src/plugin/documentation/documentation-service.ts(1 hunks)packages/renderer/src/lib/dialogs/CommandPalette.svelte(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/main/src/assets/fallback-documentation.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/api/src/documentation-info.ts
- packages/main/src/plugin/documentation/documentation-service.ts
- packages/main/src/plugin/documentation/documentation-service.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: smoke e2e tests (production)
- GitHub Check: macOS
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: typecheck
- GitHub Check: build website
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Sourcery review
| onMount(async () => { | ||
| const platform = await window.getOsPlatform(); | ||
| isMac = platform === 'darwin'; | ||
| documentationItems = await window.getDocumentationItems(); | ||
| }); |
There was a problem hiding this comment.
Guard both preload APIs to prevent mount failures.
Both getOsPlatform() and getDocumentationItems() can throw if the preload bridge is unavailable. The previous review flagged getOsPlatform() but the same issue affects getDocumentationItems().
Apply this diff:
onMount(async () => {
- const platform = await window.getOsPlatform();
- isMac = platform === 'darwin';
- documentationItems = await window.getDocumentationItems();
+ try {
+ const platform = await window.getOsPlatform();
+ isMac = platform === 'darwin';
+ } catch (e) {
+ console.warn('getOsPlatform unavailable; defaulting to non-mac', e);
+ isMac = false;
+ }
+
+ try {
+ documentationItems = await window.getDocumentationItems();
+ } catch (e) {
+ console.warn('getDocumentationItems unavailable; skipping docs', e);
+ documentationItems = [];
+ }
});📝 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.
| onMount(async () => { | |
| const platform = await window.getOsPlatform(); | |
| isMac = platform === 'darwin'; | |
| documentationItems = await window.getDocumentationItems(); | |
| }); | |
| onMount(async () => { | |
| try { | |
| const platform = await window.getOsPlatform(); | |
| isMac = platform === 'darwin'; | |
| } catch (e) { | |
| console.warn('getOsPlatform unavailable; defaulting to non-mac', e); | |
| isMac = false; | |
| } | |
| try { | |
| documentationItems = await window.getDocumentationItems(); | |
| } catch (e) { | |
| console.warn('getDocumentationItems unavailable; skipping docs', e); | |
| documentationItems = []; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/renderer/src/lib/dialogs/CommandPalette.svelte around lines 102 to
106, both preload bridge calls (window.getOsPlatform() and
window.getDocumentationItems()) must be guarded to avoid mount failures when the
preload API is unavailable; wrap each call in existence checks and try/catch (or
a single try/catch around both) and fall back to safe defaults (e.g., isMac =
false and documentationItems = []), ensuring any caught error is swallowed or
logged but does not abort component mount.
7024e88 to
f25ab9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (4)
227-254: Use explicit type guards for more reliable item discrimination.The current discriminator (
'category' in item) works but is fragile—if CommandInfo ever gains a category property, this breaks. Consider using a more explicit check.Use a type guard that checks for documentation-specific properties:
async function executeAction(index: number): Promise<void> { const item = filteredItems[index]; if (!item) return; - // Check if it's a documentation item by checking for 'category' property - const isDocItem = 'category' 475E in item; + // Check if it's a documentation item by checking for url property (docs have urls, commands have ids) + const isDocItem = 'url' in item && typeof (item as any).url === 'string'; if (isDocItem) { // Documentation item const docItem = item as DocumentationInfo; if (docItem.url) { try { await window.openExternal(docItem.url); } catch (error) { console.error('Error opening documentation URL', error); } } } else { // Command item const commandItem = item as CommandInfo; if (commandItem.id) { try { await window.executeCommand(commandItem.id); } catch (error) { console.error('error executing command', error); } } } }
102-106: Guard preload APIs to prevent mount failures.Both
window.getOsPlatform()andwindow.getDocumentationItems()can throw if the preload bridge is unavailable, causing the component to fail mounting.Wrap each call in a try-catch with safe defaults:
onMount(async () => { - const platform = await window.getOsPlatform(); - isMac = platform === 'darwin'; - documentationItems = await window.getDocumentationItems(); + try { + const platform = await window.getOsPlatform(); + isMac = platform === 'darwin'; + } catch (e) { + console.warn('getOsPlatform unavailable; defaulting to non-mac', e); + isMac = false; + } + + try { + documentationItems = await window.getDocumentationItems(); + } catch (e) { + console.warn('getDocumentationItems unavailable; skipping docs', e); + documentationItems = []; + } });
74-83: Fix null-safe filtering to prevent runtime errors.The optional chaining stops at
toLowerCase(), but calling.includes()on undefined will throw when fields are missing.Chain through includes or use nullish coalescing:
let filteredDocumentationInfoItems: DocumentationInfo[] = $derived( documentationItems.filter(item => inputValue - ? item.name?.toLowerCase().includes(inputValue.toLowerCase()) || - item.description?.toLowerCase().includes(inputValue.toLowerCase()) || - item.category?.toLowerCase().includes(inputValue.toLowerCase()) + ? (item.name ?? '').toLowerCase().includes(inputValue.toLowerCase()) || + (item.description ?? '').toLowerCase().includes(inputValue.toLowerCase()) || + (item.category ?? '').toLowerCase().includes(inputValue.toLowerCase()) : true, ), );
237-243: Validate URL scheme before opening external links.Opening external URLs without validation could pose security risks if malicious URLs are injected into documentation items.
Add URL validation to restrict to safe schemes:
if (isDocItem) { // Documentation item const docItem = item as DocumentationInfo; if (docItem.url) { try { - await window.openExternal(docItem.url); + const url = new URL(docItem.url); + if (url.protocol === 'http:' || url.protocol === 'https:') { + await window.openExternal(url.toString()); + } else { + console.warn('Blocked non-http(s) documentation URL:', docItem.url); + } } catch (error) { console.error('Error opening documentation URL', error); } } }packages/main/src/plugin/documentation/documentation-service.ts (2)
65-70: Add concurrent call deduplication and return defensive copy.Multiple simultaneous calls to
getDocumentationItems()will each triggerfetchDocumentation(), wasting network resources. Additionally, returningthis.documentationdirectly exposes the internal array to mutation.Implement call deduplication and defensive copying:
export class DocumentationService extends Disposable { private documentation: DocumentationInfo[] = []; private isInitialized = false; + private initPromise?: Promise<void>; constructor( @inject(ApiSenderType) private apiSender: ApiSenderType, ) { super(() => { this.documentation = []; this.isInitialized = false; + this.initPromise = undefined; }); } async getDocumentationItems(): Promise<DocumentationInfo[]> { if (!this.isInitialized) { - await this.fetchDocumentation(); + this.initPromise ??= this.fetchDocumentation().finally(() => { + this.initPromise = undefined; + }); + await this.initPromise; } - return this.documentation; + return [...this.documentation]; } async refreshDocumentation(): Promise<void> { this.isInitialized = false; // Force re-fetch + this.initPromise = undefined; await this.fetchDocumentation(); this.apiSender.send('documentation-updated'); }
78-100: Add timeout to prevent hanging fetch requests.The code handles
AbortError(lines 93-94) but never sets a timeout. Network requests can hang indefinitely if the remote server is unresponsive.Implement timeout using AbortController:
private async fetchJsonContent(url: string): Promise<DocumentationBaseInfo[]> { try { - const response = await fetch(url); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 8000); + + const response = await fetch(url, { + signal: controller.signal, + headers: { 'Accept': 'application/json' } + }); + clearTimeout(timeoutId); if (!response.ok) { throw new Error(`HTTP ${response.status}: ${response.statusText}`); } const json = await response.json(); if (!Array.isArray(json)) { throw new Error(`Invalid JSON format for ${url}`); } return json; } catch (error: unknown) { if (error instanceof Error) { if (error.name === 'AbortError') { throw new Error(`Request timeout while fetching ${url}`); } throw new Error(`Failed to fetch ${url}: ${error.message}`); } throw new Error(`Failed to fetch ${url}: Unknown error`); } }packages/main/src/plugin/documentation/documentation-service.spec.ts (1)
64-73: Critical test mock mismatch prevents happy-path coverage.The service implementation (documentation-service.ts lines 78-90) calls
response.json(), but these mocks provide only atext()method. When the tests run,response.jsonis undefined, causing the service to throw and fall back to offline data. This means the fetch/parse success path is never exercised.Update all fetch mocks to return
json()instead oftext():const fetchSpy = vi .spyOn(global, 'fetch') .mockResolvedValueOnce({ ok: true, - text: () => Promise.resolve(mockDocsHtml), + json: () => Promise.resolve([ + { name: 'Introduction', url: '/docs/intro' }, + { name: 'Containers Guide', url: '/docs/containers' }, + { name: 'Kubernetes Guide', url: '/docs/kubernetes' } + ]), } as Response) .mockResolvedValueOnce({ ok: true, - text: () => Promise.resolve(mockTutorialHtml), + json: () => Promise.resolve([ + { name: 'Getting Started Tutorial', url: '/tutorial/getting-started' }, + { name: 'Kubernetes Cluster Tutorial', url: '/tutorial/kubernetes-cluster' } + ]), } as Response);Apply the same fix to all similar mocks throughout the test file (lines 127-136, 150-159, 175-184, 237-246, 290-299, 318-327, 349-358, 380-389).
🧹 Nitpick comments (3)
packages/main/src/plugin/documentation/documentation-service.ts (3)
133-133: Consider simpler ID generation if IDs are internal-only.Using SHA-256 for ID generation (line 133) is cryptographically secure but may be overkill if IDs are only used internally. A simpler approach like slugifying the name or using a counter would be faster and more readable.
If IDs aren't exposed to users or used for security-sensitive C462 purposes, simplify:
-id: createHash('sha256').update(item.name).digest('hex'), +id: `${config.category.toLowerCase()}-${item.name.toLowerCase().replace(/[^a-z0-9]+/g, '-')}`,Or use a counter for guaranteed uniqueness:
let idCounter = 0; // ... id: `${config.category.toLowerCase()}-${idCounter++}`,
60-60: Consider runtime validation of fallback data structure.The fallback documentation is cast to
DocumentationInfo[](lines 60, 111, 149) without runtime validation. While the JSON structure currently matches the type, runtime validation would catch future mismatches.Add a simple validation helper:
private validateDocumentationItem(item: unknown): item is DocumentationInfo { return ( typeof item === 'object' && item !== null && 'id' in item && typeof (item as any).id === 'string' && 'name' in item && typeof (item as any).name === 'string' && 'description' in item && typeof (item as any).description === 'string' && 'url' in item && typeof (item as any).url === 'string' && 'category' in item && typeof (item as any).category === 'string' ); } // Then use it: const validated = (fallbackDocumentation as unknown[]).filter(this.validateDocumentationItem); if (validated.length !== fallbackDocumentation.length) { console.warn('Some fallback documentation items failed validation'); } this.documentation = validated;
109-112: Input validation is adequate but could be more thorough.The current validation checks for missing parameters and array types but doesn't validate field types or URL formats within items.
For production robustness, consider adding deeper validation:
private validateDocumentationBaseInfo(item: any): item is DocumentationBaseInfo { return ( typeof item?.name === 'string' && item.name.length > 0 && typeof item?.url === 'string' && (item.url.startsWith('http://') || item.url.startsWith('https://') || item.url.startsWith('/')) ); } // Then in parseDocumentationFromJson: for (const item of config.data) { if (this.validateDocumentationBaseInfo(item)) { // ... create DocumentationInfo } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/api/src/documentation-info.ts(1 hunks)packages/main/src/assets/fallback-documentation.json(1 hunks)packages/main/src/plugin/documentation/documentation-service.spec.ts(1 hunks)packages/main/src/plugin/documentation/documentation-service.ts(1 hunks)packages/renderer/src/lib/dialogs/CommandPalette.svelte(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/main/src/plugin/documentation/documentation-service.ts (2)
packages/extension-api/src/extension-api.d.ts (1)
Disposable(90-118)packages/api/src/documentation-info.ts (2)
DocumentationInfo(24-28)DocumentationBaseInfo(19-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Sourcery review
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke e2e tests (development)
- GitHub Check: smoke e2e tests (production)
- GitHub Check: typecheck
- GitHub Check: linter, formatters
- GitHub Check: Linux
- GitHub Check: build website
- GitHub Check: macOS
- GitHub Check: Windows
🔇 Additional comments (1)
packages/main/src/assets/fallback-documentation.json (1)
1-44: LGTM!The fallback documentation structure correctly implements the DocumentationInfo interface with all required fields (id, name, description, url, category). The entries provide a sensible minimal set covering core documentation areas, and all URLs are absolute and properly formatted.
|
@jiridostal @axel7083 PR updated |
| <div class='flex grow items-center flex-col gap-2 py-4'> | ||
| <Icon icon={NotFoundIcon} /> | ||
| <div class='text-lg font-bold'>No results matching '{inputValue}' found</div> | ||
| <div class='text-md'>Not what you expected? Double-check your spelling or try searching for:</div> |
There was a problem hiding this comment.
I know it's a nitpick... but the message without any button or link after : looks like a bug 🤷 :-D
There was a problem hiding this comment.
Updated, should now be visible only when docs is selected
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
f25ab9a to
5d1d45b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (1)
346-367: Align item rendering discriminator with executeAction.The item rendering uses the same unreliable discriminator (
'category' in item) asexecuteAction. Once you fix the discriminator inexecuteAction, update this rendering logic to use the same reliable check for consistency.Apply this diff after fixing
executeAction:- {@const isDocItem = 'category' in item} + {@const isDocItem = 'url' in item && typeof (item as any).url === 'string'}
♻️ Duplicate comments (6)
packages/main/src/plugin/documentation/documentation-service.ts (3)
72-76: Verify ApiSenderType.send signature and add payload if required.The
apiSender.sendcall passes only the channel name. Previous reviews indicated thatApiSenderType.sendexpects a payload argument. If the signature requires two parameters, add an appropriate payload (e.g.,undefinedor a status object).If the signature requires a payload, apply this diff:
- this.apiSender.send('documentation-updated'); + this.apiSender.send('documentation-updated', undefined);Run this script to verify the
ApiSenderType.sendsignature:#!/bin/bash # Check ApiSenderType.send signature ast-grep --pattern $'class $_ { $$$ send($_, $_) { $$$ } $$$ }'
44-63: Add request timeout to prevent hanging network calls.The
fetchDocumentationmethod lacks a timeout on its fetch operations, which can cause the service to hang indefinitely if the network is slow or unresponsive. This was flagged in previous reviews but remains unaddressed.Apply this diff to add timeout handling:
async fetchDocumentation(): Promise<void> { try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 8000); + const [docsJson, tutorialsJson] = await Promise.all([ - this.fetchJsonContent('https://podman-desktop.io/docs.json'), - this.fetchJsonContent('https://podman-desktop.io/tutorials.json'), + this.fetchJsonContent('https://podman-desktop.io/docs.json', controller.signal), + this.fetchJsonContent('https://podman-desktop.io/tutorials.json', controller.signal), ]); + clearTimeout(timeoutId); if (docsJson && tutorialsJson) { this.documentation = this.parseDocumentationFromJson(docsJson, tutorialsJson); } else { throw new Error('Failed to fetch documentation JSON files'); } this.isInitialized = true; } catch (error: unknown) { console.error('Failed to fetch documentation at startup:', error); // Fallback to predefined documentation if fetching fails this.documentation = fallbackDocumentation as DocumentationInfo[]; this.isInitialized = true; } }Update
fetchJsonContentsignature to accept the signal:-private async fetchJsonContent(url: string): Promise<DocumentationBaseInfo[]> { +private async fetchJsonContent(url: string, signal?: AbortSignal): Promise<DocumentationBaseInfo[]> { try { - const response = await fetch(url); + const response = await fetch(url, { signal });
65-70: Cache the initial load and return a defensive copy.The method re-fetches on every call when not initialized and returns the internal array by reference, allowing external mutation. Additionally, concurrent calls can trigger multiple fetches. This was flagged in previous reviews.
Apply this diff to add caching and defensive copying:
export class DocumentationService extends Disposable { private documentation: DocumentationInfo[] = []; private isInitialized = false; + private loadPromise?: Promise<void>; constructor( @inject(ApiSenderType) private apiSender: ApiSenderType, ) { super(() => { this.documentation = []; this.isInitialized = false; + this.loadPromise = undefined; }); } - async getDocumentationItems(): Promise<DocumentationInfo[]> { - if (!this.isInitialized) { - await this.fetchDocumentation(); - } - return this.documentation; - } + async getDocumentationItems(): Promise<DocumentationInfo[]> { + if (!this.isInitialized) { + this.loadPromise ??= this.fetchDocumentation().finally(() => { + this.loadPromise = undefined; + }); + await this.loadPromise; + } + return [...this.documentation]; + } async refreshDocumentation(): Promise<void> { - this.isInitialized = false; + this.loadPromise = undefined; + this.isInitialized = false; await this.fetchDocumentation(); this.apiSender.send('documentation-updated'); }packages/renderer/src/lib/dialogs/CommandPalette.svelte (3)
102-106: Add error handling to prevent mount failures.Both
window.getOsPlatform()andwindow.getDocumentationItems()can throw if the preload bridge is unavailable, causing the component mount to fail. This issue was flagged in previous reviews but remains unaddressed.Apply this diff to guard both calls:
onMount(async () => { - const platform = await window.getOsPlatform(); - isMac = platform === 'darwin'; - documentationItems = await window.getDocumentationItems(); + try { + const platform = await window.getOsPlatform(); + isMac = platform === 'darwin'; + } catch (e) { + console.warn('getOsPlatform unavailable; defaulting to non-mac', e); + isMac = false; + } + + try { + documentationItems = await window.getDocumentationItems(); + } catch (e) { + console.warn('getDocumentationItems unavailable; using empty array', e); + documentationItems = []; + } });
74-83: Fix null-safe filtering to prevent runtime errors.The optional chaining stops at
toLowerCase(), but.includes()is still called on potentially undefined values, which will throw. This was flagged in previous reviews.Apply this diff:
let filteredDocumentationInfoItems: DocumentationInfo[] = $derived( documentationItems.filter(item => inputValue - ? item.name?.toLowerCase().includes(inputValue.toLowerCase()) || - item.description?.toLowerCase().includes(inputValue.toLowerCase()) || - item.category?.toLowerCase().includes(inputValue.toLowerCase()) + ? item.name?.toLowerCase()?.includes(inputValue.toLowerCase()) || + item.description?.toLowerCase()?.includes(inputValue.toLowerCase()) || + item.category?.toLowerCase()?.includes(inputValue.toLowerCase()) : true, ), );
227-254: Use a reliable discriminator and add safety guards.The current discriminator (
'category' in item) is unreliable becauseCommandInfomight also have acategoryproperty, leading to misclassification. Additionally, the preload bridge methods are called without existence checks, and URL schemes are not validated.Apply this diff:
async function executeAction(index: number): Promise<void> { const item = filteredItems[index]; if (!item) return; - // Check if it's a documentation item by checking for 'category' property - const isDocItem = 'category' in item; + // Discriminate by presence of a URL; commands have id but not url + const isDocItem = 'url' in item && typeof (item as any).url === 'string'; if (isDocItem) { // Documentation item const docItem = item as DocumentationInfo; - if (docItem.url) { - try { - await window.openExternal(docItem.url); - } catch (error) { - console.error('Error opening documentation URL', error); - } - } + if (!docItem.url) return; + try { + const u = new URL(docItem.url); + if (u.protocol === 'http:' || u.protocol === 'https:') { + if (typeof window.openExternal === 'function') { + await window.openExternal(u.toString()); + } else { + console.warn('openExternal unavailable; skipping'); + } + } else { + console.warn('Blocked non-http(s) URL:', docItem.url); + } + } catch (error) { + console.error('Invalid documentation URL', error); + } } else { // Command item const commandItem = item as CommandInfo; - if (commandItem.id) { - try { - await window.executeCommand(commandItem.id); - } catch (error) { - console.error('error executing command', error); - } - } + if (!commandItem.id) return; + try { + if (typeof window.executeCommand === 'function') { + await window.executeCommand(commandItem.id); + } else { + console.warn('executeCommand unavailable; skipping'); + } + } catch (error) { + console.error('error executing command', error); + } } }
🧹 Nitpick comments (1)
packages/main/src/plugin/documentation/documentation-service.ts (1)
128-144: Consider simpler ID generation.Using SHA-256 to hash item names for IDs is functional but potentially excessive. Since IDs are not user-visible (based on past review context), a simpler approach like a counter or a lightweight hash would suffice and improve readability.
Consider this simpler approach:
+ private idCounter = 0; + for (const config of parseConfigs) { try { for (const item of config.data) { if (item.name && item.url) { documentation.push({ - id: createHash('sha256').update(item.name).digest('hex'), + id: `${config.category.toLowerCase()}-${++this.idCounter}`, name: item.name, description: `${config.category}: ${item.name}`, url: item.url, category: config.category, }); } }Or use a lightweight hash like
djb2:private simpleHash(str: string): string { let hash = 5381; for (let i = 0; i < str.length; i++) { hash = ((hash << 5) + hash) + str.charCodeAt(i); } return (hash >>> 0).toString(36); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/api/src/documentation-info.ts(1 hunks)packages/main/src/assets/fallback-documentation.json(1 hunks)packages/main/src/plugin/documentation/documentation-service.spec.ts(1 hunks)packages/main/src/plugin/documentation/documentation-service.ts(1 hunks)packages/renderer/src/lib/dialogs/CommandPalette.svelte(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/main/src/plugin/documentation/documentation-service.spec.ts
- packages/api/src/documentation-info.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/main/src/plugin/documentation/documentation-service.ts (2)
packages/extension-api/src/extension-api.d.ts (1)
Disposable(90-118)packages/api/src/documentation-info.ts (2)
DocumentationInfo(24-28)DocumentationBaseInfo(19-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: smoke e2e tests (production)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: typecheck
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: unit tests / windows-2025
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: build website
- GitHub Check: macOS
- GitHub Check: Sourcery review
🔇 Additional comments (2)
packages/main/src/assets/fallback-documentation.json (1)
1-44: LGTM! Fallback data structure is correct.The JSON structure correctly matches the
DocumentationInfointerface with all required fields (id, name, description, url, category). The entries provide a reasonable offline fallback covering key documentation areas.Note: These hardcoded URLs may require periodic updates to stay aligned with the website structure.
packages/main/src/plugin/documentation/documentation-service.ts (1)
29-154: Overall architecture is sound.The service properly extends
Disposable, handles initialization state, implements fallback gracefully, and provides a clean API surface. The separation of concerns between fetching, parsing, and fallback is well-structured.
What does this PR do?
This PR adds service that fetches content from our website from docs and tutorial
And shows the content in searchbar under docs tab
Screenshot / video of UI
What issues does this PR fix or reference?
Closes #13660
How to test this PR?