fix #3605 #3618

taylordowns2000 · 2025-09-27T10:28:01Z

Description

This PR fixes #3605

Validation steps

Open the AI chat on an existing workflow
Try to send without any text.
Note the send button is grey
Note that you can still attempt to send via command+enter
Note that the error message indicates you need to enter some text
Note that the canvas doesn't freeze up

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

You can read more details in our Responsible AI Policy

Pre-submission checklist

I have performed a self-review of my code.
I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
I have updated the changelog.
I have ticked a box in "AI usage" in this PR

codecov · 2025-09-27T11:12:35Z

Codecov Report

❌ Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.60%. Comparing base (6adbdd3) to head (2cbdcd6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/lightning_web/live/ai_assistant/component.ex	57.14%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3618      +/-   ##
==========================================
- Coverage   89.65%   89.60%   -0.05%     
==========================================
  Files         410      410              
  Lines       17075    17086      +11     
==========================================
+ Hits        15308    15310       +2     
- Misses       1767     1776       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

elias-ba

Hey @taylordowns2000 great work on this one. I have few observations. They should probably not block this PR from a functionality point of view. But I was thinking they could improve code quality. Let me know if you wanna discuss them

elias-ba · 2025-09-27T12:43:53Z

lib/lightning_web/live/ai_assistant/modes/job_code.ex

+      |> validate_required([:content],
+        message: "Please enter a message before sending"
+      )
+      |> validate_length(:content,
+        min: 1,
+        message: "Please enter a message before sending"
+      )


Is both validate_required and validate_length(min: 1) necessary here? They seem to cover the same case (empty content). Could we simplify this to just one validation?

elias-ba · 2025-09-27T12:57:14Z

lib/lightning_web/live/ai_assistant/component.ex

        socket
      ) do
    cleared_params = Map.put(params, "content", nil)
+    trimmed_content = if is_binary(content), do: String.trim(content), else: ""


Since content comes from a form input, it should always be a string. The is_binary check is likely unnecessary.

elias-ba · 2025-09-27T12:58:45Z

lib/lightning_web/live/ai_assistant/component.ex

+      trimmed_content == "" ->
+        changeset = socket.assigns.handler.validate_form(%{"content" => ""})
+
+        changeset =
+          Ecto.Changeset.add_error(
+            changeset,
+            :content,
+            "Please enter a message before sending"
+          )
+
+        {:noreply,
+         socket
+         |> assign(
+           changeset: changeset,
+           alert: "Please enter a message before sending"
+         )}


This manual empty content validation duplicates what should already be handled by the changeset validations. The current approach creates inconsistency where some validation happens in changesets and some happens in the controller. Why are we not relying entirely on changeset validation ?

elias-ba · 2025-09-27T13:58:25Z

lib/lightning_web/live/ai_assistant/component.ex

            id={"ai-assistant-form-submit-btn-#{@id}"}
            type="submit"
-            disabled={@disabled}
+            disabled={@disabled || form_content_empty?(@form[:content].value)}


Why isn't this part of the @disabled attribute. When the button is disabled other elements of this component should be disabled too, right ?

hey @elias-ba , no, i don't think so. unless im misunderstanding what you're saying here. have you really tested this hard in the user interface? what you're asking here would be bad—we want the whole thing to be enabled but we want the submit button to be disabled. if we disab. ed the entire form when the user hasn't yet typed antyhing into the chat, you'd never be able to type anything into the chat.

have i misunderstood your request? sorry if so!

elias-ba · 2025-09-27T13:58:49Z

lib/lightning_web/live/ai_assistant/component.ex

            class={[
              "p-1.5 rounded-full focus:outline-none focus:ring-2 focus:ring-offset-2 transition-all duration-200 flex items-center justify-center h-7 w-7",
-              if(@disabled,
+              if(@disabled || form_content_empty?(@form[:content].value),


Same observation here ?

elias-ba · 2025-09-27T14:00:11Z

lib/lightning_web/live/ai_assistant/mode_behavior.ex

+          |> validate_required([:content],
+            message: "Please enter a message before sending"
+          )
+          |> validate_length(:content,
+            min: 1,
+            message: "Please enter a message before sending"
+          )


Same observation than before about the duplication of this validation.

taylordowns2000 · 2025-09-29T20:40:16Z

hey @elias-ba , i re-requested your review because I implemented the changes you requested but in doing so realized that the feature no longer works as desired. all changes reverted. if this functionality worked as expected in the UI when you tested it, i'd recommend approving. (you mentioned in your PR that this is merely code quality stuff, and this code will be thrown with the elixir AI chat—replaced by JS—very soon anyway!)

They should probably not block this PR from a functionality point of view. But I was thinking they could improve code quality. Let me know if you wanna discuss them

And btw, if there's something in particular that feels like it should block this, let me know!

elias-ba

@taylordowns2000 you are right we shouldn't block this PR for small code quality stuff. There's nothing in here that's a bad design. Just few stuff that caught my attention. I am going to approve this and we can always come back later and make code better. Congratulations for the great work 👏🏽

mercy-ship-it · 2025-10-01T06:49:42Z

@elias-ba @taylordowns2000 I’ve reviewed the updates and I don’t see anything that should block this PR from being merged.

Everything looks fine, and since this will be replaced by the JS version soon, no need to block over concerns issues—we can clean those up later.

elias-ba · 2025-10-01T06:58:06Z

Thanks a lot @mercy-ship-it, happy we aligned. I think you need to approve the PR.

fix #3605

47ef0dd

github-project-automation bot added this to v2 Sep 27, 2025

github-project-automation bot moved this to New Issues in v2 Sep 27, 2025

taylordowns2000 requested review from elias-ba and mercy-ship-it September 27, 2025 10:28

taylordowns2000 added 4 commits September 27, 2025 12:32

tests

fd32413

cleaner diff

8cc8e1f

format

a35d9c9

update existing tests

61373f2

elias-ba requested changes Sep 27, 2025

View reviewed changes

github-project-automation bot moved this from New Issues to In review in v2 Sep 27, 2025

taylordowns2000 requested a review from elias-ba September 28, 2025 09:39

taylordowns2000 force-pushed the 3605 branch from 2120921 to 61373f2 Compare September 28, 2025 10:04

elias-ba approved these changes Oct 1, 2025

View reviewed changes

mercy-ship-it approved these changes Oct 1, 2025

View reviewed changes

Merge branch 'main' into 3605

2cbdcd6

taylordowns2000 merged commit f30ca39 into main Oct 1, 2025
6 of 8 checks passed

taylordowns2000 deleted the 3605 branch October 1, 2025 12:06

github-project-automation bot moved this from In review to Done in v2 Oct 1, 2025

fix #3605 #3618

fix #3605 #3618

Uh oh!

Conversation

taylordowns2000 commented Sep 27, 2025

Description

Validation steps

AI Usage

Pre-submission checklist

Uh oh!

codecov bot commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

elias-ba left a comment

Choose a reason for hiding this comment

Uh oh!

elias-ba Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

elias-ba Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

elias-ba Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

elias-ba Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

taylordowns2000 Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

elias-ba Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

elias-ba Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

taylordowns2000 commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elias-ba left a comment

Choose a reason for hiding this comment

Uh oh!

mercy-ship-it commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elias-ba commented Oct 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 27, 2025 •

edited

Loading

taylordowns2000 commented Sep 29, 2025 •

edited

Loading

mercy-ship-it commented Oct 1, 2025 •

edited

Loading