Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Aug 26, 2025

PR Type

Enhancement


Description

  • Refactor instruction testing page to use agent-based workflow

  • Update event handlers and component interfaces

  • Improve error handling and user feedback

  • Clean up commented code and unused functionality


Diagram Walkthrough

flowchart LR
  A["User Interface"] --> B["Agent Selection"]
  B --> C["Template Selection"]
  C --> D["LLM Configuration"]
  D --> E["Execute Instruction"]
  E --> F["Display Results"]
Loading

File Walkthrough

Relevant files
Enhancement
instruction-agent.svelte
Update agent selection component                                                 

src/routes/page/instruction/instruction-components/instruction-agent.svelte

  • Remove filter for agents with templates
  • Change event name from agentSelected to selectAgent
  • Remove commented HTML select code
+2/-14   
instruction-llm.svelte
Refactor LLM configuration component                                         

src/routes/page/instruction/instruction-components/instruction-llm.svelte

  • Change selectedModel type from any to string?
  • Rename function from collectLlmOptions to initLlmOptions
  • Add initialization logic to reset selections
  • Update event name from llmSelected to selectLlm
  • Simplify model selection logic
+14/-13 
+page.svelte
Refactor testing page for agent workflow                                 

src/routes/page/instruction/testing/+page.svelte

  • Replace sendChatCompletion with executeAgentInstruction
  • Add error handling for missing agent selection
  • Update tab from 'Template' to 'Agent'
  • Add tooltip component for user guidance
  • Disable instruction textarea and remove manual editing
  • Update event handler names to match component changes
+39/-22 
instructTypes.js
Update instruction type definitions                                           

src/lib/helpers/types/instructTypes.js

  • Add text property to InstructMessageModel
  • Update type annotations for optional properties
  • Add states property for conversation state
+5/-1     
instruct-service.js
Update instruction service interface                                         

src/lib/services/instruct-service.js

  • Update function to return response data
  • Add return type annotation
  • Rename parameter from instruction to request
+6/-4     

@qodo-merge-pro
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The instruction textarea is now permanently disabled and the request always sends instruction as '#TEMPLATE#'. If the agent/template is required, ensure UX reflects this and that free-form instruction is not needed; otherwise users cannot test custom prompts anymore.

<Row class="instruct-setting-container">
    <Col lg="7">
        <div>
            <div class="instruct-setting-section" style="gap: 2px;">
                <textarea
                    class='form-control knowledge-textarea'
                    rows={19}
                    maxlength={maxLength}
                    disabled
                    placeholder={''}
                    bind:value={instruction}
                />
                <!-- <div class="text-secondary text-end text-count">
                    <div>{instruction?.length || 0}/{maxLength}</div>
                </div> -->
            </div>
        </div>
    </Col>
    <Col lg="5" class="instruction-gap">
        <Card>
            <CardBody>
                <NavBar
                    id={'instruction-nav-container'}
                    disableDefaultStyles
                    containerClasses={'nav-tabs-secondary'}
                >
                    {#each tabs as tab, idx}
                    <NavItem
                        containerStyles={`flex: 0 1 calc(100% / ${tabs.length <= 2 ? tabs.length : 3})`}
                        navBtnStyles={'text-transform: none;'}
                        navBtnId={`${tab.name}-tab`}
State Reset Risk

initLlmOptions resets selectedProvider and selectedModel whenever llmConfigs changes/reacts, which may unexpectedly clear user selections after initial load or on any reactive change.

$: {
    initLlmOptions(llmConfigs);
}

/** @param {import('$commonTypes').LlmConfig[]} llmConfigs */
function initLlmOptions(llmConfigs) {
    selectedProvider = null;
    selectedModel = null;
    providerOptions = [];
    modelOptions = [];

    providerOptions = llmConfigs?.map(x => ({
        label: x.provider,
        value: x.provider
    }))?.sort((a, b) => a.label.localeCompare(b.label)) || [];
}
Behavior Change

Removed filter for agents with templates; UI may now allow selecting agents without templates. Verify downstream flows handle agents lacking templates and that templateOptions/content are robust to empty cases.

function collectAgentOptions(agents) {
    agentOptions = agents?.map(x => ({
        label: x.name,
        value: x.id
    }))?.sort((a, b) => a.label.localeCompare(b.label)) || [];
}

@iceljc iceljc merged commit c2e10ee into SciSharp:main Aug 26, 2025
1 of 2 checks passed
@qodo-merge-pro
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Send template as string

The API type now declares template as a string, but an object is being sent.
Pass the template's identifier or name to avoid server-side parsing errors. Use
a safe optional access to prevent undefined access.

src/routes/page/instruction/testing/+page.svelte [100-107]

 executeAgentInstruction(agentId, {
             text: util.trim(text) || '',
             instruction: '#TEMPLATE#',
             provider: selectedProvider?.provider,
             model: selectedModel,
-            template: selectedTemplate,
+            template: selectedTemplate?.name || null,
             states: formattedStates
         }).then(res => {
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that an object selectedTemplate is passed where a string is expected for the template property, preventing a potential server-side error.

Medium
Reset model on provider change

When the provider changes without a targetModel, the previously selected model
(from another provider) may be retained, causing an invalid provider/model pair.
Reset the model to null to ensure consistency.

src/routes/page/instruction/instruction-components/instruction-llm.svelte [74-77]

 if (!!targetModel) {
             selectedModel = modelOptions.find(x => x.value === targetModel)?.value || null;
+        } else {
+            selectedModel = null;
         }
     }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that selectedModel is not reset when the provider changes without a targetModel, which could lead to an inconsistent state.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant