Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

launcher block #1948

Merged
merged 10 commits into from
Feb 12, 2025
Merged

launcher block #1948

merged 10 commits into from
Feb 12, 2025

Conversation

sawka
Copy link
Member

@sawka sawka commented Feb 11, 2025

No description provided.

Copy link
Contributor

coderabbitai bot commented Feb 11, 2025

Walkthrough

The pull request introduces multiple changes across the codebase. A new launcher view has been added, including a corresponding view model and component registration in the block registry. Modifications in the block frame component now conditionally render the error boundary based on a new state variable. In the global store, an asynchronous function to replace existing blocks has been implemented, and functions handling new block creation and splits have been refactored to use a centralized block definition retrieval mechanism based on a settings key. Layout tree manipulation functions now update focus management by incorporating a focused property. Additionally, the settings configuration in both JSON and Go files, along with associated TypeScript and schema definitions, has been expanded to include a new default block option. A guide for the view model system is also introduced to document the structure and usage of interactive blocks and their UI components.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09dff23 and d37e897.

📒 Files selected for processing (1)
  • frontend/layout/lib/layoutTree.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/layout/lib/layoutTree.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: merge-gatekeeper
  • GitHub Check: Analyze (go)
  • GitHub Check: Build Docsite
  • GitHub Check: Build for TestDriver.ai

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
frontend/app/view/view-prompt.md (1)

222-240: 🛠️ Refactor suggestion

Remove "Instructions to AI" section.

This section appears to be internal documentation and should be removed from the user-facing guide.

-## Instructions to AI
-
-1. Generate a new **ViewModel** class for a block, following the structure above.
-2. Generate a corresponding **ViewComponent**.
-3. Use **Jotai atoms** to store all dynamic state.
-4. Ensure the ViewModel defines **header elements** (`viewText`, `viewIcon`, `endIconButtons`).
-5. Export the view model (to be registered in the BlockRegistry)
-6. Use existing metadata patterns for config and settings.
-
-## Other Notes
-
-- The types you see above don't need to be imported, they are global types (custom.d.ts)
-
-**Output Format:**
-
-- TypeScript code defining the **ViewModel**.
-- TypeScript code defining the **ViewComponent**.
-- Ensure alignment with the patterns in `waveai.tsx`, `preview.tsx`, `sysinfo.tsx`, and `term.tsx`.
🧹 Nitpick comments (7)
frontend/app/view/launcher/launcher.tsx (3)

24-26: Avoid forward reference to the LauncherView component.
Referencing a component before it is declared can cause lint or runtime issues in certain configurations. Consider moving the LauncherView declaration before this property assignment or place the model class in a separate file.

- viewComponent = LauncherView;
...
+ // Option A: Move LauncherView definition above this class
+ // OR
+ // Option B: Export and import LauncherView in a separate file, then reference it here.
🧰 Tools
🪛 Biome (1.9.4)

[error] 26-26: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)


56-66: Remove or lower-log the console debug statement.
The console.log("keydown", e); is helpful for debugging, but could be noisy in production. Consider removing or gating behind a debug flag.

- console.log("keydown", e);
+ // console.debug("keydown", e);

111-117: Surface async errors to end-users.
Right now, if replaceBlock fails, the only feedback is logged in the console. Consider exposing user-facing feedback or toast notifications.

async handleWidgetSelect(widget: WidgetConfigType) {
  try {
    await replaceBlock(this.blockId, widget.blockdef);
  } catch (error) {
    console.error("Error replacing block:", error);
+   // Possibly show a user-facing error here
  }
}
pkg/wconfig/settingsconfig.go (1)

55-55: LGTM! Consider adding documentation.

The new field follows the established naming conventions and tagging patterns. Consider adding a comment to document the purpose and expected values of this setting.

+    // AppDefaultNewBlock specifies the default block type to use when creating new blocks.
+    // Example values: "term", "web", etc.
     AppDefaultNewBlock string `json:"app:defaultnewblock,omitempty"`
frontend/app/block/blockframe.tsx (1)

540-541: LGTM! Consider using a more explicit conditional.

The conditional rendering logic is correct but could be more explicit for better readability.

-                {noHeader || <ErrorBoundary fallback={headerElemNoView}>{headerElem}</ErrorBoundary>}
+                {!noHeader && <ErrorBoundary fallback={headerElemNoView}>{headerElem}</ErrorBoundary>}

Also applies to: 623-623

frontend/app/store/global.ts (1)

454-471: LGTM! Consider enhancing error handling.

The function follows the established patterns and correctly handles block replacement. Consider adding try-catch blocks for async operations and cleanup on failure.

 async function replaceBlock(blockId: string, blockDef: BlockDef): Promise<string> {
     const tabId = globalStore.get(atoms.staticTabId);
     const layoutModel = getLayoutModelForTabById(tabId);
     const rtOpts: RuntimeOpts = { termsize: { rows: 25, cols: 80 } };
-    const newBlockId = await ObjectService.CreateBlock(blockDef, rtOpts);
-    const targetNodeId = layoutModel.getNodeByBlockId(blockId)?.id;
-    if (targetNodeId == null) {
-        throw new Error(`targetNodeId not found for blockId: ${blockId}`);
-    }
-    const replaceNodeAction: LayoutTreeReplaceNodeAction = {
-        type: LayoutTreeActionType.ReplaceNode,
-        targetNodeId: targetNodeId,
-        newNode: newLayoutNode(undefined, undefined, undefined, { blockId: newBlockId }),
-        focused: true,
-    };
-    layoutModel.treeReducer(replaceNodeAction);
-    return newBlockId;
+    try {
+        const newBlockId = await ObjectService.CreateBlock(blockDef, rtOpts);
+        const targetNodeId = layoutModel.getNodeByBlockId(blockId)?.id;
+        if (targetNodeId == null) {
+            throw new Error(`targetNodeId not found for blockId: ${blockId}`);
+        }
+        const replaceNodeAction: LayoutTreeReplaceNodeAction = {
+            type: LayoutTreeActionType.ReplaceNode,
+            targetNodeId: targetNodeId,
+            newNode: newLayoutNode(undefined, undefined, undefined, { blockId: newBlockId }),
+            focused: true,
+        };
+        layoutModel.treeReducer(replaceNodeAction);
+        return newBlockId;
+    } catch (error) {
+        console.error('Error replacing block:', error);
+        throw error;
+    }
 }
frontend/app/view/view-prompt.md (1)

64-64: Fix typo in color name.

There's a space in "text-su cess" which should be "text-success".

-   - Colors are also defined for error, warning, and success (text-error, text-warning, text-su cess)
+   - Colors are also defined for error, warning, and success (text-error, text-warning, text-success)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d36bd38 and 024b980.

📒 Files selected for processing (14)
  • frontend/app/block/block.tsx (2 hunks)
  • frontend/app/block/blockframe.tsx (2 hunks)
  • frontend/app/store/global.ts (3 hunks)
  • frontend/app/store/keymodel.ts (3 hunks)
  • frontend/app/view/launcher/launcher.tsx (1 hunks)
  • frontend/app/view/view-prompt.md (1 hunks)
  • frontend/layout/lib/layoutTree.ts (1 hunks)
  • frontend/layout/lib/types.ts (1 hunks)
  • frontend/types/custom.d.ts (1 hunks)
  • frontend/types/gotypes.d.ts (1 hunks)
  • pkg/wconfig/defaultconfig/settings.json (1 hunks)
  • pkg/wconfig/metaconsts.go (1 hunks)
  • pkg/wconfig/settingsconfig.go (1 hunks)
  • schema/settings.json (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/app/view/launcher/launcher.tsx

[error] 26-26: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)

🪛 LanguageTool
frontend/app/view/view-prompt.md

[uncategorized] ~14-~14: Loose punctuation mark.
Context: ...block type identifier. - viewIcon, viewName, viewText: Atoms for UI me...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~15-~15: Loose punctuation mark.
Context: ... for UI metadata. - preIconButton, endIconButtons: Atoms for action butt...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~16-~16: Loose punctuation mark.
Context: ...oms for action buttons. - blockBg: Atom for background styling. - `ma...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~17-~17: Loose punctuation mark.
Context: ...round styling. - manageConnection, noPadding, searchAtoms. - `vie...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~18-~18: Loose punctuation mark.
Context: ..., searchAtoms. - viewComponent`: React component rendering the block. ...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~56-~56: Loose punctuation mark.
Context: ... - useOverrideConfigAtom(blockId, key): Reads from global config but allows per...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...rrides. - useSettingsKeyAtom(key): Accesses global settings efficiently. ...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: Build for TestDriver.ai
🔇 Additional comments (14)
frontend/app/view/launcher/launcher.tsx (4)

12-17: Sorting logic confirmed to be correct.
No issues with the sorting strategy; code is concise and effectively handles missing or undefined display order via the nullish coalescing operator.


38-46: Search filter atom usage looks good.
The logic to exclude hidden widgets and filter by label is straightforward and well-organized.


159-181: Dynamic grid layout approach is well-designed.
The iterative calculation of columns and tile sizes to find the best fit is a neat solution, yielding a responsive grid.


193-264: Clean and accessible rendering structure.
The layout, hidden search input for screen readers, and final instructions are well done for keyboard and accessible navigation.

pkg/wconfig/metaconsts.go (1)

12-12: Consistent addition of new config key.
This new constant follows the existing naming pattern and furthers the configuration extensibility.

frontend/layout/lib/types.ts (1)

198-198: Optional focused property is a sensible extension.
Adding the focused?: boolean field across these action interfaces aligns with typical focus management scenarios.

Also applies to: 209-209, 218-218

frontend/app/block/block.tsx (1)

12-12: LGTM! Block registry extended with launcher view.

The launcher view model is properly imported and registered in the BlockRegistry following the established pattern.

Also applies to: 48-48

frontend/types/custom.d.ts (1)

285-286: LGTM! ViewModel interface extended with header visibility control.

The noHeader property is properly typed as an optional jotai atom and well-documented.

frontend/app/store/keymodel.ts (2)

180-212: LGTM! Well-structured default block definition retrieval.

The function properly handles both launcher and terminal cases while preserving context from the focused block.


214-237: LGTM! Command handlers refactored to use centralized block definition logic.

The changes improve code maintainability by centralizing block definition retrieval across all block creation paths.

frontend/layout/lib/layoutTree.ts (1)

453-455: LGTM! Focus management added to replaceNode.

The change maintains consistency with focus behavior in other node manipulation functions.

frontend/types/gotypes.d.ts (1)

684-684: LGTM!

The new property follows the established patterns for settings properties.

pkg/wconfig/defaultconfig/settings.json (1)

6-6: LGTM! The new configuration entry follows the established pattern.

The new setting app:defaultnewblock is well-positioned and follows the naming convention for app-specific settings.

schema/settings.json (1)

17-19: LGTM! The schema definition aligns with the configuration.

The new property app:defaultnewblock is correctly defined as a string type and properly positioned within the app-related settings section.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
docs/docs/keybindings.mdx (1)

63-71: Consider adding more details about bookmark functionality.

The new bookmark keybinding (<Kbd k="Cmd:o"/>) is correctly added, but consider expanding the description to mention that it opens the bookmark selector/picker for better user understanding.

docs/docs/config.mdx (1)

126-199: Consider these minor improvements to the WebBookmarks documentation.

The WebBookmarks Configuration section is comprehensive, but could benefit from these minor improvements:

  1. Add a comma after "By convention" in the introduction.
  2. Rephrase the sorting behavior description to: "Bookmarks are sorted based on display:order (if provided); otherwise, they follow the default order."
  3. Consider changing "anywhere in your computer" to "anywhere on your computer" in the global hotkey section.

These changes would improve readability while maintaining the excellent technical content.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~128-~128: Possible missing comma found.
Context: ... file (bookmarks.json) as a key-value map where the key (id) is an arbitrary id...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~128-~128: A comma is probably missing here.
Context: ...bitrary identifier for the bookmark. By convention you should start your ids with "bookmar...

(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)


[typographical] ~191-~191: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ed based on display:order (if provided), otherwise they follow default order. - icon and...

(THUS_SENTENCE)


[uncategorized] ~191-~191: You might be missing the article “the” here.
Context: ...r(if provided), otherwise they follow default order. -iconandiconcolor` are rar...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 024b980 and 9d82eb9.

📒 Files selected for processing (2)
  • docs/docs/config.mdx (4 hunks)
  • docs/docs/keybindings.mdx (2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/docs/config.mdx

[uncategorized] ~128-~128: Possible missing comma found.
Context: ... file (bookmarks.json) as a key-value map where the key (id) is an arbitrary id...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~128-~128: A comma is probably missing here.
Context: ...bitrary identifier for the bookmark. By convention you should start your ids with "bookmar...

(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)


[typographical] ~191-~191: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ed based on display:order (if provided), otherwise they follow default order. - icon and...

(THUS_SENTENCE)


[uncategorized] ~191-~191: You might be missing the article “the” here.
Context: ...r(if provided), otherwise they follow default order. -iconandiconcolor` are rar...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~285-~285: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ...n your most recent window from anywhere in your computer. This has the name `"app:...

(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Build for TestDriver.ai
  • GitHub Check: Build Docsite
  • GitHub Check: merge-gatekeeper
  • GitHub Check: Analyze (go)
🔇 Additional comments (2)
docs/docs/keybindings.mdx (1)

23-44: LGTM! The global keybindings table is well-organized and clear.

The updates to the global keybindings table are comprehensive and well-documented, particularly:

  • Clear description of the app:defaultnewblock setting for <Kbd k="Cmd:n"/>.
  • Logical placement of new split operations (<Kbd k="Cmd:d"/> and <Kbd k="Cmd:Shift:d"/>).
docs/docs/config.mdx (1)

30-30: LGTM! Clear documentation of the new configuration key.

The app:defaultnewblock configuration key is well-documented with clear options and default value.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (8)
docs/docs/config.mdx (1)

287-287: Fix preposition in the sentence.

The sentence reads awkwardly with "in your computer".

-from anywhere in your computer
+from anywhere on your computer
🧰 Tools
🪛 LanguageTool

[uncategorized] ~287-~287: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ...n your most recent window from anywhere in your computer. This has the name `"app:...

(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)

frontend/app/view/launcher/launcher.tsx (3)

12-17: Add type safety for widget sorting.

The sortByDisplayOrder function could benefit from improved type safety and validation.

Consider this safer implementation:

-function sortByDisplayOrder(wmap: { [key: string]: WidgetConfigType } | null | undefined): WidgetConfigType[] {
+function sortByDisplayOrder(wmap: Record<string, WidgetConfigType> | null | undefined): WidgetConfigType[] {
     if (!wmap) return [];
     const wlist = Object.values(wmap);
-    wlist.sort((a, b) => (a["display:order"] ?? 0) - (b["display:order"] ?? 0));
+    wlist.sort((a, b) => {
+        const orderA = typeof a["display:order"] === 'number' ? a["display:order"] : 0;
+        const orderB = typeof b["display:order"] === 'number' ? b["display:order"] : 0;
+        return orderA - orderB;
+    });
     return wlist;
 }

56-123: Refactor keyboard handler to reduce code duplication.

The keyDownHandler method has repeated patterns for handling empty widget lists and index updates.

Consider extracting common logic:

keyDownHandler(e: WaveKeyboardEvent): boolean {
    if (this.gridLayout == null) {
        return false;
    }
+    const handleEmptyList = () => {
+        return filteredWidgets.length === 0;
+    };
+
+    const updateIndex = (newIndex: number) => {
+        if (newIndex >= 0 && newIndex < filteredWidgets.length) {
+            globalStore.set(this.selectedIndex, newIndex);
+        }
+    };

    const gridLayout = this.gridLayout;
    const filteredWidgets = globalStore.get(this.filteredWidgetsAtom);
    const selectedIndex = globalStore.get(this.selectedIndex);
    // ... rest of the implementation using the helper functions
}

134-279: Enhance component performance and accessibility.

The component could benefit from several improvements:

  1. Memoize event handlers
  2. Extract grid layout logic
  3. Enhance accessibility

Consider these improvements:

 const LauncherView: React.FC<ViewComponentProps<LauncherViewModel>> = ({ blockId, model }) => {
+    // Memoize handlers
+    const handleSearchChange = React.useCallback((e: React.ChangeEvent<HTMLInputElement>) => {
+        setSearchTerm(e.target.value);
+    }, [setSearchTerm]);
+
+    const handleWidgetClick = React.useCallback((widget: WidgetConfigType) => {
+        model.handleWidgetSelect(widget);
+    }, [model]);

     // ... rest of the component

     return (
         <div 
             ref={containerRef} 
             className="w-full h-full p-4 box-border flex flex-col items-center justify-center"
+            role="dialog"
+            aria-label="Widget Launcher"
         >
             <input
                 ref={model.inputRef}
                 type="text"
                 value={searchTerm}
                 onKeyDown={keydownWrapper(model.keyDownHandler.bind(model))}
-                onChange={(e) => setSearchTerm(e.target.value)}
+                onChange={handleSearchChange}
                 className="sr-only"
                 aria-label="Search widgets"
+                aria-expanded="true"
+                aria-controls="widget-grid"
             />

             {/* ... Logo section ... */}

             <div
                 className="grid gap-4 justify-center"
+                id="widget-grid"
+                role="grid"
                 style={{
                     gridTemplateColumns: `repeat(${gridLayout.columns}, ${finalTileWidth}px)`,
                 }}
             >
                 {filteredWidgets.map((widget, index) => (
                     <div
                         key={index}
-                        onClick={() => model.handleWidgetSelect(widget)}
+                        onClick={() => handleWidgetClick(widget)}
+                        role="gridcell"
+                        aria-selected={index === selectedIndex}
                         // ... rest of the widget cell
                     >
                         {/* ... widget content ... */}
                     </div>
                 ))}
             </div>
         </div>
     );
 };

Also, consider extracting the grid layout calculation into a custom hook:

function useGridLayout(containerSize: Size, availableHeight: number, itemCount: number) {
    return React.useMemo(() => {
        // ... existing grid layout calculation logic
    }, [containerSize, availableHeight, itemCount]);
}
frontend/app/view/view-prompt.md (4)

59-64: Enhance styling guidelines with a structured format.

The styling guidelines could be more organized and easier to reference.

Consider restructuring the styling section like this:

-7. **Styling**
-   - Use TailWind CSS to style components
-   - Accent color is: text-accent, for a 50% transparent accent background use bg-accentbg
-   - Hover background is: bg-hoverbg
-   - Border color is "border", so use border-border
-   - Colors are also defined for error, warning, and success (text-error, text-warning, text-sucess)
+7. **Styling Guidelines**
+   
+   Wave Terminal uses TailWind CSS for styling components.
+   
+   ### Color Classes
+   | Purpose | Class Name | Usage |
+   |---------|------------|-------|
+   | Accent | `text-accent` | Primary accent color |
+   | Accent Background | `bg-accentbg` | 50% transparent accent background |
+   | Hover | `bg-hoverbg` | Background color on hover |
+   | Border | `border-border` | Standard border color |
+   
+   ### Status Colors
+   | Status | Class Name |
+   |--------|------------|
+   | Error | `text-error` |
+   | Warning | `text-warning` |
+   | Success | `text-success` |

54-58: Add example usage for helper functions.

The helper functions section would be more helpful with concrete examples.

Consider adding examples like this:

// Example: Reading and updating block-specific metadata
const fontSizeAtom = useBlockMetaKeyAtom(blockId, "fontSize");
const fontSize = useAtomValue(fontSizeAtom);
useSetAtom(fontSizeAtom)("14px");

// Example: Using global config with block override
const themeAtom = useOverrideConfigAtom(blockId, "theme");
const theme = useAtomValue(themeAtom);

// Example: Accessing global settings
const globalFontAtom = useSettingsKeyAtom("defaultFont");
const globalFont = useAtomValue(globalFontAtom);
🧰 Tools
🪛 LanguageTool

[uncategorized] ~56-~56: Loose punctuation mark.
Context: ... - useOverrideConfigAtom(blockId, key): Reads from global config but allows per...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~56-~56: Possible missing comma found.
Context: ...blockId, key): Reads from global config but allows per-block overrides. - use...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...rrides. - useSettingsKeyAtom(key): Accesses global settings efficiently. ...

(UNLIKELY_OPENING_PUNCTUATION)


202-219: Enhance example with more features.

The current example is minimal. Consider expanding it to demonstrate more features of the ViewModel system.

Here's an enhanced example:

import * as jotai from "jotai";
import React from "react";

class HelloWorldModel implements ViewModel {
    viewType = "helloworld";
    viewIcon = jotai.atom("smile");
    viewName = jotai.atom("Hello World");
    viewText = jotai.atom("A simple greeting block");
    
    // Add interactive elements
    nameAtom = jotai.atom("World");
    endIconButtons = jotai.atom([
        {
            elemtype: "iconbutton",
            icon: "refresh",
            click: () => this.resetName()
        }
    ]);
    
    // Add header menu
    menuItems: MenuItem[] = [
        {
            label: "Clear",
            icon: "trash",
            onClick: () => this.resetName()
        }
    ];
    
    viewComponent = HelloWorldView;
    
    resetName() {
        jotai.set(this.nameAtom, "World");
    }
}

const HelloWorldView: ViewComponent<HelloWorldModel> = ({ model }) => {
    const [name, setName] = useAtom(model.nameAtom);
    
    return (
        <div className="p-4 flex flex-col gap-2">
            <input
                value={name}
                onChange={(e) => setName(e.target.value)}
                className="border border-border rounded px-2 py-1"
            />
            <div className="text-accent">
                Hello, {name}!
            </div>
        </div>
    );
};

export { HelloWorldModel };

231-240: Add testing and error handling guidelines.

The notes section could benefit from additional information about testing and error handling.

Consider adding these sections:

 ## Other Notes
 
 - The types you see above don't need to be imported, they are global types (custom.d.ts)
+
+### Testing Guidelines
+- Create unit tests for your ViewModel logic
+- Test edge cases in state management
+- Verify header element interactions
+- Test keyboard event handling
+
+### Error Handling
+- Implement proper error boundaries
+- Handle async operation failures gracefully
+- Provide user-friendly error messages
+- Consider network connectivity issues
 
 **Output Format:**
 
 - TypeScript code defining the **ViewModel**.
 - TypeScript code defining the **ViewComponent**.
 - Ensure alignment with the patterns in `waveai.tsx`, `preview.tsx`, `sysinfo.tsx`, and `term.tsx`.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d82eb9 and 09dff23.

📒 Files selected for processing (3)
  • docs/docs/config.mdx (6 hunks)
  • frontend/app/view/launcher/launcher.tsx (1 hunks)
  • frontend/app/view/view-prompt.md (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/app/view/launcher/launcher.tsx

[error] 26-26: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)

🪛 LanguageTool
docs/docs/config.mdx

[uncategorized] ~287-~287: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ...n your most recent window from anywhere in your computer. This has the name `"app:...

(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)

frontend/app/view/view-prompt.md

[uncategorized] ~14-~14: Loose punctuation mark.
Context: ...block type identifier. - viewIcon, viewName, viewText: Atoms for UI me...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~15-~15: Loose punctuation mark.
Context: ... for UI metadata. - preIconButton, endIconButtons: Atoms for action butt...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~16-~16: Loose punctuation mark.
Context: ...oms for action buttons. - blockBg: Atom for background styling. - `ma...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~17-~17: Loose punctuation mark.
Context: ...round styling. - manageConnection, noPadding, searchAtoms. - `vie...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~18-~18: Loose punctuation mark.
Context: ..., searchAtoms. - viewComponent`: React component rendering the block. ...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~56-~56: Loose punctuation mark.
Context: ... - useOverrideConfigAtom(blockId, key): Reads from global config but allows per...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~56-~56: Possible missing comma found.
Context: ...blockId, key): Reads from global config but allows per-block overrides. - use...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...rrides. - useSettingsKeyAtom(key): Accesses global settings efficiently. ...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: merge-gatekeeper
  • GitHub Check: Build for TestDriver.ai
  • GitHub Check: Build Docsite
🔇 Additional comments (4)
docs/docs/config.mdx (4)

7-14: LGTM! Platform-specific documentation improvements.

The addition of platform context components enhances the documentation by providing platform-specific content rendering capabilities.

Also applies to: 334-334


38-38: LGTM! Clear documentation of the new launcher block feature.

The new configuration key app:defaultnewblock is well-documented with clear options and default value.

Also applies to: 103-103


134-202: LGTM! Comprehensive WebBookmarks documentation.

The new WebBookmarks section is well-structured with:

  • Clear explanation of the feature
  • Detailed field descriptions
  • Practical example
  • Behavioral notes

289-295: LGTM! Improved section organization.

The reorganization of the global hotkey documentation with clear examples and allowed key names improves readability.

Comment on lines +125 to +131
async handleWidgetSelect(widget: WidgetConfigType) {
try {
await replaceBlock(this.blockId, widget.blockdef);
} catch (error) {
console.error("Error replacing block:", error);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling in widget selection.

The error handling in handleWidgetSelect could be more informative and user-friendly.

Consider this enhanced implementation:

 async handleWidgetSelect(widget: WidgetConfigType) {
     try {
         await replaceBlock(this.blockId, widget.blockdef);
     } catch (error) {
-        console.error("Error replacing block:", error);
+        const errorMessage = error instanceof Error ? error.message : String(error);
+        console.error(`Failed to replace block ${this.blockId} with widget ${widget.label}:`, errorMessage);
+        // Consider adding user feedback here
     }
 }
📝 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.

Suggested change
async handleWidgetSelect(widget: WidgetConfigType) {
try {
await replaceBlock(this.blockId, widget.blockdef);
} catch (error) {
console.error("Error replacing block:", error);
}
}
async handleWidgetSelect(widget: WidgetConfigType) {
try {
await replaceBlock(this.blockId, widget.blockdef);
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error);
console.error(`Failed to replace block ${this.blockId} with widget ${widget.label}:`, errorMessage);
// Consider adding user feedback here
}
}

Comment on lines +21 to +33
export class LauncherViewModel implements ViewModel {
blockId: string;
viewType = "launcher";
viewIcon = atom("shapes");
viewName = atom("Widget Launcher");
viewComponent = LauncherView;
noHeader = atom(true);
inputRef = { current: null } as React.RefObject<HTMLInputElement>;
searchTerm = atom("");
selectedIndex = atom(0);
containerSize = atom({ width: 0, height: 0 });
gridLayout: GridLayoutType = null;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix circular reference in view model initialization.

The viewComponent property references LauncherView before it's declared, which could cause issues in some bundlers.

Move the viewComponent assignment after the LauncherView declaration:

export class LauncherViewModel implements ViewModel {
    blockId: string;
    viewType = "launcher";
    viewIcon = atom("shapes");
    viewName = atom("Widget Launcher");
-    viewComponent = LauncherView;
    noHeader = atom(true);
    inputRef = { current: null } as React.RefObject<HTMLInputElement>;
    searchTerm = atom("");
    selectedIndex = atom(0);
    containerSize = atom({ width: 0, height: 0 });
    gridLayout: GridLayoutType = null;
}

// ... LauncherView component definition ...

// Add this after LauncherView is defined
LauncherViewModel.prototype.viewComponent = LauncherView;
🧰 Tools
🪛 Biome (1.9.4)

[error] 26-26: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)

Comment on lines +78 to +93
interface ViewModel {
viewType: string;
viewIcon?: jotai.Atom<string | IconButtonDecl>;
viewName?: jotai.Atom<string>;
viewText?: jotai.Atom<string | HeaderElem[]>;
preIconButton?: jotai.Atom<IconButtonDecl>;
endIconButtons?: jotai.Atom<IconButtonDecl[]>;
blockBg?: jotai.Atom<MetaType>;
manageConnection?: jotai.Atom<boolean>;
noPadding?: jotai.Atom<boolean>;
searchAtoms?: SearchAtoms;
viewComponent: ViewComponent;
dispose?: () => void;
giveFocus?: () => boolean;
keyDownHandler?: (e: WaveKeyboardEvent) => boolean;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add missing noHeader property and enhance type documentation.

The ViewModel interface is missing the noHeader property mentioned in the changes, and the interface could benefit from JSDoc comments.

Add the missing property and enhance documentation:

 interface ViewModel {
+  /** Unique identifier for the block type */
   viewType: string;
+  /** Icon displayed in the block header */
   viewIcon?: jotai.Atom<string | IconButtonDecl>;
+  /** Block name displayed in the header */
   viewName?: jotai.Atom<string>;
+  /** Text or elements displayed in the header */
   viewText?: jotai.Atom<string | HeaderElem[]>;
+  /** Icon button displayed at the start of the header */
   preIconButton?: jotai.Atom<IconButtonDecl>;
+  /** Icon buttons displayed at the end of the header */
   endIconButtons?: jotai.Atom<IconButtonDecl[]>;
+  /** Background styling for the block */
   blockBg?: jotai.Atom<MetaType>;
+  /** Controls connection management UI */
   manageConnection?: jotai.Atom<boolean>;
+  /** Disables default padding */
   noPadding?: jotai.Atom<boolean>;
+  /** Controls header visibility */
+  noHeader?: jotai.Atom<boolean>;
+  /** Search-related atoms */
   searchAtoms?: SearchAtoms;
+  /** React component that renders the block */
   viewComponent: ViewComponent;
+  /** Cleanup function called when block is disposed */
   dispose?: () => void;
+  /** Focus handler returning true if focus was handled */
   giveFocus?: () => boolean;
+  /** Keyboard event handler returning true if event was handled */
   keyDownHandler?: (e: WaveKeyboardEvent) => boolean;
 }
📝 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.

Suggested change
interface ViewModel {
viewType: string;
viewIcon?: jotai.Atom<string | IconButtonDecl>;
viewName?: jotai.Atom<string>;
viewText?: jotai.Atom<string | HeaderElem[]>;
preIconButton?: jotai.Atom<IconButtonDecl>;
endIconButtons?: jotai.Atom<IconButtonDecl[]>;
blockBg?: jotai.Atom<MetaType>;
manageConnection?: jotai.Atom<boolean>;
noPadding?: jotai.Atom<boolean>;
searchAtoms?: SearchAtoms;
viewComponent: ViewComponent;
dispose?: () => void;
giveFocus?: () => boolean;
keyDownHandler?: (e: WaveKeyboardEvent) => boolean;
}
interface ViewModel {
/** Unique identifier for the block type */
viewType: string;
/** Icon displayed in the block header */
viewIcon?: jotai.Atom<string | IconButtonDecl>;
/** Block name displayed in the header */
viewName?: jotai.Atom<string>;
/** Text or elements displayed in the header */
viewText?: jotai.Atom<string | HeaderElem[]>;
/** Icon button displayed at the start of the header */
preIconButton?: jotai.Atom<IconButtonDecl>;
/** Icon buttons displayed at the end of the header */
endIconButtons?: jotai.Atom<IconButtonDecl[]>;
/** Background styling for the block */
blockBg?: jotai.Atom<MetaType>;
/** Controls connection management UI */
manageConnection?: jotai.Atom<boolean>;
/** Disables default padding */
noPadding?: jotai.Atom<boolean>;
/** Controls header visibility */
noHeader?: jotai.Atom<boolean>;
/** Search-related atoms */
searchAtoms?: SearchAtoms;
/** React component that renders the block */
viewComponent: ViewComponent;
/** Cleanup function called when block is disposed */
dispose?: () => void;
/** Focus handler returning true if focus was handled */
giveFocus?: () => boolean;
/** Keyboard event handler returning true if event was handled */
keyDownHandler?: (e: WaveKeyboardEvent) => boolean;
}

@sawka sawka merged commit 539559c into main Feb 12, 2025
10 checks passed
@sawka sawka deleted the sawka/block-prompt branch February 12, 2025 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant