Skip to content

Conversation

@AdityaHegde
Copy link
Collaborator

@AdityaHegde AdityaHegde commented Nov 27, 2025

Adds CTA to generate sample data using AI. There is on in the welcome screen and add asset under more in file explorer.

Closes APP-607

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!
@nishantmonu51 nishantmonu51 added the blocker A release blocker issue that should be resolved before a new release label Nov 28, 2025
@AdityaHegde AdityaHegde self-assigned this Dec 1, 2025
@AdityaHegde AdityaHegde marked this pull request as ready for review December 1, 2025 05:31
@AdityaHegde AdityaHegde force-pushed the feat/rill-dev-generate-data branch from 1c77908 to 0df155f Compare December 1, 2025 06:13
@ericpgreen2
Copy link
Contributor

Concerns with generateModel function boundary and error handling

File: web-common/src/features/chat/core/actions.ts:22

1. Function Boundary / Encapsulation Issue

The function signature suggests it generates a model, but the caller is responsible for constructing model-specific prompts:

// Caller in GenerateSampleData.svelte
void generateModel(
  isInit,
  instanceId,
  `Generate a model for the following user prompt: ${prompt}`,
);

Suggestion: The function should encapsulate the prompt construction and use clearer parameter names:

// Rename for clarity
export async function generateSampleData(
  initializeProject: boolean,  // Clearer than "isInit"
  instanceId: string,
  businessContext: string,     // Raw user input
) {
  // Construct agent prompt internally
  const agentPrompt = `Generate a model for the following user prompt: ${businessContext}`;
  // ...
  conversation.draftMessage.set(agentPrompt);
}

This prevents leaky abstractions and makes the API more maintainable.


2. Silent Error Handling

Multiple catch blocks swallow errors without providing user feedback:

try {
  fileCreated(callMsg, "Data already present at");
} catch {
  // no-op - line 96
}
} catch {
  overlay.set(null);  // line 133
}

Impact: Users see generic "Failed to generate sample data" regardless of whether the issue was:

  • Project initialization timeout
  • AI generation failure
  • Cancellation
  • File system error

Suggestion: Log errors and provide specific user feedback:

} catch (error) {
  console.error("Sample data generation failed:", error);
  overlay.set(null);
  eventBus.emit("notification", {
    message: cancelled ? "Generation cancelled" : "Failed to generate sample data. Please try again.",
    type: "error"
  });
}

3. Premature Overlay Dismissal

await conversation.sendMessage({}, { onMessage: handleMessage });
await waitUntil(() => get(conversation.isStreaming));

overlay.set(null);  // ← Dismissed as soon as streaming starts!

The overlay is cleared when streaming starts, not when it completes. If file creation happens later in the stream, users won't see any loading indicator.

Suggestion: Wait for completion or handle in the message callback:

await waitUntil(() => !get(conversation.isStreaming));  // Wait for completion
if (!cancelled && !created) {
  overlay.set(null);
  // Show error...
}

4. Project Init Timeout

const PROJECT_INIT_TIMEOUT_MS = 10_000;  // 10 seconds

const projectResetPromise = new Promise<void>((resolve, reject) => {
  eventBus.once("project-reset", () => resolve());
  setTimeout(reject, PROJECT_INIT_TIMEOUT_MS);
});

Concerns:

  • 10 seconds may be too short on slower machines
  • Timeout rejection is caught silently—users won't know initialization failed
  • No cleanup if function exits early (potential memory leak for event listener)

Suggestion:

  • Increase timeout to 30s or make configurable
  • Provide specific timeout error message
  • Add cleanup: eventBus.off() in finally block or use AbortController pattern

Overall, the core logic is sound, but error handling and user experience need strengthening before this goes to production. Happy to discuss any of these points!


Developed in collaboration with Claude Code

@ericpgreen2
Copy link
Contributor

GenerateSampleData component should use the form library

File: web-common/src/features/welcome/GenerateSampleData.svelte

Current Issues

The component currently uses a basic textarea with button onClick handler, which has several UX problems:

  1. No validation - Users can submit empty prompts
  2. No keyboard support - Enter key doesn't submit (expected behavior for text inputs)
  3. Inconsistent with codebase - Other dialogs with user input use svelte-forms-lib
  4. Missing accessibility - No proper form semantics

Recommendation

Use svelte-forms-lib with yup validation, matching the pattern in BookmarksFormDialog.svelte and other forms in the codebase:

<script lang="ts">
  import { createForm } from "svelte-forms-lib";
  import * as yup from "yup";
  
  const { form, errors, handleSubmit } = createForm({
    initialValues: {
      prompt: "",
    },
    validationSchema: yup.object({
      prompt: yup
        .string()
        .required("Please describe your data")
        .min(10, "Please provide more detail (at least 10 characters)"),
    }),
    onSubmit: async (values) => {
      void generateModel(isInit, instanceId, values.prompt);
      open = false;
    },
  });
</script>

<form on:submit|preventDefault={handleSubmit}>
  <textarea
    bind:value={$form.prompt}
    placeholder="e.g. sales transaction of an e-commerce store"
  />
  {#if $errors.prompt}
    <span class="error">{$errors.prompt}</span>
  {/if}
  
  <Button type="primary" large onClick={handleSubmit}>
    Generate
  </Button>
</form>

Benefits

Validation - Prevents empty/too-short prompts
Enter key - Natural form submission behavior
Error messages - Clear user feedback when validation fails
Consistency - Matches existing form patterns
Better UX - Can disable button while form is invalid

Even for a single field, the improved UX and consistency with the codebase make this worthwhile.


Developed in collaboration with Claude Code

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

UXQA:

  1. I'd expect enter would submit the form.
  2. After submitting the AI request, I pressed "cancel" in the UI and nothing happened. The cancellation didn't seem to work, as I waited and the AI generated a valid model.
  3. After creating sample data through the Welcome screen, I tried creating more sample data through the "Add > More > Sample Data" option. I used a slightly different prompt. My first file was overwritten. To be safe, we should always generate a new file, not overwrite any existing files.
  4. Upon returning to the "Add > More > Sample Data" dialog, I'd expect my prior prompt to have been cleared.
  5. Upon the data being created, I see two notifications: the dialog and the toast notification. We should only have one. Let's remove the toast notification.
    Image
  6. I deleted the models directory, and I tried to generate more sample data. Nothing happened. The generation errored, yet I didn't see any error notification. I see this error in the event stream: "open /models/hello_world.yaml no such file or directory"
@ericokuma
Copy link
Contributor

I'm trying to test this but everytime I kick off a sample data generation request, it leads to a "failed to generate sample data" toast notification

@AdityaHegde
Copy link
Collaborator Author

Thanks @ericpgreen2 . I have addressed your comments. Regarding the forms library I am guessing you are talking about using superforms instead of svelte-forms-lib? Or are we going back to svelte-forms-lib?
Couldnt reproduce 2 and 6. Did you have the latest changes?

@ericokuma You need to have a valid rill cloud. Make sure to run ./rill devtool switch-env prod or start a local cloud.

@ericpgreen2
Copy link
Contributor

Regarding the forms library I am guessing you are talking about using superforms instead of svelte-forms-lib?

Yes! Apologies that part of the Claude response slipped by me.

Couldnt reproduce 2 and 6. Did you have the latest changes?

I believe I did. I'll check again.

@ericpgreen2
Copy link
Contributor

Architectural consideration: Feature-specific prompt location

Existing Pattern

Looking at how "explain anomaly" works (features/dashboards/time-series/measure-selection/measure-selection.ts):

// Prompt logic lives in the FEATURE directory
public startAnomalyExplanationChat(metricsView: string) {
  const prompt = `Explain what drives ${measureMention}, ${timeRangeMention}...`;
  sidebarActions.startChat(prompt);  // Opens chat, user sees conversation
}

Key principle: Feature-specific prompts live in their feature directories, not in chat/. The chat/ directory provides infrastructure (sidebarActions.startChat(), Conversation class), while features own their domain-specific prompt logic.

Current PR

The sample data generation logic is in chat/core/actions.ts, but it's not core chat functionality—it's a feature that uses chat as an implementation detail.

Recommendation

Move to a dedicated feature directory like features/sample-data/:

features/
  sample-data/
    generate-sample-data.ts       // Generation logic + prompt
    GenerateSampleData.svelte     // Dialog component
    OptionCancelToAIAction.svelte // Cancel UI

This keeps chat/core/ focused on chat infrastructure while allowing sample data generation to be a first-class feature with its own home.

On UX Approach

I understand the current product requirements call for an overlay-based UX (user doesn't see the conversation). That's fine for now. However, if future work aligns this with the "explain anomaly" pattern (visible conversations via sidebarActions.startChat()), having the code in features/sample-data/ will make that transition cleaner—the feature directory is the right home regardless of which chat UX is used.

For Future Prompts

As more feature-specific prompts are added (metrics views, dashboards, etc.), they should follow the same principle:

features/
  sample-data/generate-sample-data.ts
  metrics-views/generation/generate-metrics-view.ts
  canvas/generation/generate-canvas.ts
  dashboards/time-series/measure-selection.ts  // existing

This keeps domain logic with its domain, and chat as pure infrastructure.


Developed in collaboration with Claude Code

@ericpgreen2
Copy link
Contributor

Event Bus: project-reset event

Event Naming

The project-reset event is emitted whenever rill.yaml is written (WatchFilesClient.ts:63), but the name suggests a full project reset. This is slightly misleading since it fires on any rill.yaml update, not just project initialization.

Suggestion: Rename to rill-yaml-updated or similar to accurately reflect what triggers it. The current usage works, but the name could cause confusion if other code listens for "project reset" expecting different semantics.

Implicit Contract

The flow relies on an implicit contract:

  1. runtimeServiceUnpackEmpty() writes rill.yaml on the backend
  2. WatchFilesClient detects the write and emits project-reset
  3. generateSampleData resolves its promise and continues

This works given the backend constraints (no "ready" signal from UnpackEmpty), but it's worth documenting this dependency. If file watching is delayed or fails, the only feedback is a generic 10-second timeout.

Consider adding a comment in the code explaining why this event-based coordination is necessary:

// Wait for project initialization to complete.
// UnpackEmpty writes rill.yaml, which triggers WatchFilesClient to emit "project-reset"
// after invalidating queries. We need to wait for this to ensure the project is ready
// before generating sample data.
const projectResetPromise = new Promise<void>((resolve, reject) => {
  // ...
});

Developed in collaboration with Claude Code

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

UXQA:

  1. The error message's capitalization/punctuation is off between the two statements
    Image
  2. After the file is created, sometimes I see both a "Source imported successfully" dialog AND an errored source in the background. I'd expect we'd only see this "success" message once the model is error-free.
    Image
@AdityaHegde AdityaHegde force-pushed the feat/rill-dev-generate-data branch from 083ad06 to f770d79 Compare December 2, 2025 15:24
Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

I'm good with the PR, as long as my above UXQA item 2 is addressed. Additionally, we discussed putting a "Beta" tag next to the feature CTAs.

I'll leave @nishantmonu51 to approve the PR tomorrow AM IST.

Comment on lines 59 to 66

if args.Create {
entries, err := t.Runtime.ListFiles(ctx, session.InstanceID(), args.Path+"*")
if err != nil {
return nil, err
}
args.Path = fileutil.GetPath(args.Path, entries)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case for this?

The Path passed to DevelopModel should ideally already be a correct .yaml path. If the developer_agent doesn't set that correctly, we should just return a useful error so it can self-correct. And if it does set it correctly, then I don't think we need the Create flag – it's already implied in the description for Path:

Path string `json:"path" jsonschema:"The path of a .yaml file in which to create or update a Rill model definition."`

note "in which to create or update"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updating the prompt doesnt guarantee that the AI doesnt overwrite existing models, that is why I added this as a safeguard. Since we are tagging this as beta ok to not worry too much? cc @nishantmonu51

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@AdityaHegde AdityaHegde dismissed begelundmuller’s stale review December 3, 2025 14:43

Most of the backend changes have been reverted

@AdityaHegde AdityaHegde merged commit 2923ffe into main Dec 3, 2025
16 of 17 checks passed
@AdityaHegde AdityaHegde deleted the feat/rill-dev-generate-data branch December 3, 2025 14:43
AdityaHegde added a commit that referenced this pull request Dec 3, 2025
* feat: generate sample data CTA

* Add a button in add asset dropdown

* Cleanup

* PR comments

* Add safeguard for generating new models

* Use the correct form library

* Move to features/sample-data

* Improve UnpackEmpty behaviour

* Update prompt to have a unique name

* Wait until model is done compliling

* Remove the _number suffix

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

Labels

blocker A release blocker issue that should be resolved before a new release

6 participants