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

Sb/whitelabel UI #5299

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

smuun
Copy link

@smuun smuun commented Feb 14, 2025

Important

Introduces a whitelabel UI configuration system for customizable UI components, enhancing flexibility in feature visibility and functionality across the application.

  • UI Customization:
    • Introduces customUi configurations in DisplayResult.svelte, EditableSchemaForm.svelte, EditorBar.svelte, ScriptBuilder.svelte, ScriptEditor.svelte, ScriptSchema.svelte, Tooltip.svelte, and LogPanel.svelte.
    • Adds customUi prop to control visibility and functionality of UI elements like tooltips, variable pickers, and buttons.
    • Implements globalUiConfig in stores.ts to manage global UI settings.
  • Components and Features:
    • DisplayResult.svelte: Conditional rendering of AI fix slot and tooltips based on customUi.
    • EditableSchemaForm.svelte: Controls JSON view and variable picker visibility.
    • EditorBar.svelte: Manages resource picker and reset button visibility.
    • ScriptBuilder.svelte: Configures top bar and settings panel tabs.
    • ScriptEditor.svelte: Adjusts preview panel and editor bar features.
    • Tooltip.svelte: Disables tooltips globally if configured.
  • Testing and Configuration:
    • Adds a test page test_dev_page/+page.svelte to toggle between different UI configurations.
    • Defines ScriptBuilderWhitelabelCustomUi and related types in custom_ui.ts for structured UI customization.

This description was created by Ellipsis for 537c88f. It will automatically update as commits are pushed.

@smuun smuun requested a review from rubenfiszel as a code owner February 14, 2025 18:26
Copy link
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 537c88f in 3 minutes and 1 seconds

More details
  • Looked at 925 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 15 drafted comments based on config settings.
1. frontend/src/routes/test_dev_page/+page.svelte:84
  • Draft comment:
    Using a reactive assignment to update the customUi and then updating globalUiConfig inside the reactive block might introduce subtle race conditions. Consider moving the globalUiConfig update into a dedicated function or reactive store derived from customUi.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    In Svelte, reactive statements are handled in a deterministic order based on their dependencies. Since the second reactive block explicitly depends on customUi, Svelte will ensure it runs after customUi is updated. The comment seems to misunderstand how Svelte's reactive system works - there shouldn't be race conditions here.
    I could be wrong about Svelte's reactive system guarantees. Maybe there are edge cases I'm not considering.
    Svelte's documentation and implementation explicitly guarantee that reactive statements run in dependency order, making race conditions impossible in this scenario.
    The comment should be deleted because it incorrectly suggests there could be race conditions in a scenario where Svelte's reactive system prevents them by design.
2. frontend/src/routes/test_dev_page/+page.svelte:109
  • Draft comment:
    The inline script object used for the ScriptBuilder component seems hard-coded for testing. Verify if this should later be made configurable or fetched from a service to avoid potential inconsistencies in production.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to verify if the inline script object should be made configurable or fetched from a service. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules.
3. frontend/src/lib/components/custom_ui.ts:113
  • Draft comment:
    Custom UI type definitions seem comprehensive. Verify that all optional properties (e.g. 'editorBar' and 'previewPanel') are used consistently in the components to avoid type mismatches.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50%
    None
4. frontend/src/lib/components/Tooltip.svelte:18
  • Draft comment:
    Consider handling the case when globalUiConfig is not yet initialized; the use of !$globalUiConfig?.tooltips?.disabled is good, but ensure that the default behavior (e.g. tooltips enabled) is well defined.
  • Reason this comment was not posted:
    Confidence changes required: 40% <= threshold 50%
    None
5. frontend/src/lib/components/DisplayResult.svelte:432
  • Draft comment:
    Good use of customUi.aiFix flag to conditionally render the copilot fix slot. This pattern provides flexibility for whitelabeling.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
6. frontend/src/lib/components/EditableSchemaForm.svelte:57
  • Draft comment:
    Introducing the customUi prop (of type EditableSchemaFormUi) to drive JSON-only and variable picker behavior is a nice extension for whitelabeling.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
7. frontend/src/lib/components/EditorBar.svelte:85
  • Draft comment:
    Conditional rendering for resource picker based on customUi.resource != false is clear. Consider verifying consistency of boolean checks throughout.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
8. frontend/src/lib/components/ScriptBuilder.svelte:172
  • Draft comment:
    Filtering the script languages using customUi.settingsPanel.metadata.languages is a good approach. Ensure the supported language list is kept in sync with documentation.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
9. frontend/src/lib/components/ScriptSchema.svelte:15
  • Draft comment:
    Pass the customUi property to EditableSchemaForm so that whitelabel options apply to schema editing; this enhances consistency with new UI configuration.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
10. frontend/src/lib/components/Tooltip.svelte:18
  • Draft comment:
    Using $globalUiConfig to disable tooltips globally is a clean solution. Verify that the scale transformation computed from customSize works across different UI themes.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
11. frontend/src/lib/components/custom_ui.ts:39
  • Draft comment:
    The new whitelabel type definitions provide granular control. Consider adding inline comments or documentation to describe each property for future maintainers.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
12. frontend/src/lib/components/schema/SchemaFormDND.svelte:23
  • Draft comment:
    Introducing the noVariablePicker prop (passed to SchemaForm) is a good addition for customizability. Ensure its behavior is tested when disabled.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
13. frontend/src/lib/components/scriptEditor/LogPanel.svelte:97
  • Draft comment:
    The conditional rendering of tabs based on customUi.history and customUi.triggerCaptures is well implemented to support custom whitelabel UI.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
14. frontend/src/lib/stores.ts:149
  • Draft comment:
    Creating a writable store for globalUiConfig is a straightforward solution to manage global UI settings. Ensure this store is updated consistently across components.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
15. frontend/src/routes/test_dev_page/+page.svelte:91
  • Draft comment:
    The example whitelabel UI configurations (no/old/new) help visualize the customization options. The reactive block updates globalUiConfig based on customUi.tooltips.disabled, which is a neat implementation.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_mXAWsqtCTPIH6ZW8


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel force-pushed the main branch 9 times, most recently from 0d0ada6 to 1ef482e Compare February 18, 2025 20:49
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