Skip to content

Ux improvements - #18

Merged
wojciech-dron merged 4 commits into
masterfrom
ux-improvements
Mar 9, 2026
Merged

wojciech-dron merged 4 commits into
masterfrom
ux-improvements

Conversation

@wojciech-dron

Copy link
Copy Markdown
Owner

No description provided.

@github-actions

github-actions Bot commented Mar 9, 2026

Copy link
Copy Markdown

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

Performance

The new modified-content change tracking calls GetModifiedValueAsync() on every editor change, which likely triggers JS interop and transfers the full editor content frequently. This can be expensive for large markdown and may degrade typing responsiveness; consider debouncing/throttling, or tracking a “dirty” flag without fetching full content on each keystroke.

private bool _hasEditorChanges;

private bool PreventNavigation => isWorking ||
    _hasEditorChanges ||
    dbContext.Entry(cvEntity).State == EntityState.Modified;

private async Task OnEditorContentChanged()
{
    if (diffEditor is null)
        return;

    var currentContent = await diffEditor.GetModifiedValueAsync();
    _hasEditorChanges = currentContent != cvEntity.MarkdownContent;
}

private async Task<bool> HasUnsavedEditorChanges()
{
    if (diffEditor is null)
        return false;

    var currentContent = await diffEditor.GetModifiedValueAsync();
    _hasEditorChanges = currentContent != cvEntity.MarkdownContent;
    return _hasEditorChanges;
}
UX Bug

The @onkeydown:preventDefault behavior is controlled by shouldPreventDefault, but that flag is set inside the keydown handler itself, which may be too late to prevent the default action for the current key event. Validate that Enter (without Shift) does not insert a newline before sending; if it does, consider an alternative approach (e.g., always prevent default and explicitly insert newline on Shift+Enter via JS, or use a JS handler for conditional preventDefault).

            <textarea class="form-control"
                      rows="4"
                      placeholder="Ask about CV or request changes..."
                      @bind="userMessage"
                      @bind:event="oninput"
                      @onkeydown="HandleKeyDown"
                      @onkeydown:preventDefault="shouldPreventDefault"
                      disabled="@IsWorking"/>
            <button type="submit" class="btn btn-primary btn-sm"
                    disabled="@(IsWorking || string.IsNullOrWhiteSpace(userMessage))">
                Send
            </button>
        </div>
    </form>
</div>

@code {
    private string userMessage = "";
    private bool shouldPreventDefault;
    private ElementReference chatContainerRef;
    private IJSObjectReference? module;

    [Parameter] public List<ChatItem> ChatHistory { get; set; } = [];
    [Parameter] public bool IsWorking { get; set; }
    [Parameter] public EventCallback<string> OnSendMessage { get; set; }

    [Inject] private IJSRuntime JS { get; set; } = default!;

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        if (ChatHistory.Count == 0)
            return;

        module ??= await JS.InvokeAsync<IJSObjectReference>("import",
            "./Features/Cv/Components/AiCvChat.razor.js");

        await module.InvokeVoidAsync("scrollToBottom", chatContainerRef);
    }

    private async Task HandleKeyDown(KeyboardEventArgs e)
    {
        if (e is { Key: "Enter", ShiftKey: false })
        {
            shouldPreventDefault = true;
            await SendMessage();
        }
        else
        {
            shouldPreventDefault = false;
        }
    }
Navigation/HTML

The new “Show PDF” link always renders and points to /cv/{id}/pdf even when the CV might not yet be persisted (id could be 0), and several places wrap a <button> inside an <a> which can cause accessibility/interaction quirks. Consider guarding the PDF link behind an id check and using an <a class="btn ..."> instead of nesting interactive elements.

<a href="/cv/@cvEntity.Id/pdf" target="_blank">
    <button class="btn btn-primary btn-sm"
            disabled="@isWorking">
        Show PDF
    </button>
</a>
<button class="btn btn-primary btn-sm"
        disabled="@isWorking"
        @onclick="GenerateCvPdf">
    Save PDF
</button>
@if (offer is not null)
{
    <button class="btn btn-primary btn-sm ms-1"
            disabled="@isWorking"
            title="Adjusts CV content with AI, with user defined rules in AI Summary"
            @onclick="PrepareCvContent">
        Adjust with AI
    </button>
}
@if (offer is not null)
{
    <a class="ms-1" href="/?offerUrl=@Uri.EscapeDataString(offer.OfferUrl)">
        <button class="btn btn-info btn-sm" disabled="@isWorking">Offer</button>
    </a>
}
<a class="ms-1" href="/cv/edit/@cvEntity.Id">
    <button class="btn btn-info btn-sm" disabled="@isWorking">
        CV page
    </button>
</a>

@github-actions

github-actions Bot commented Mar 9, 2026

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix Enter key newline prevention

@onkeydown:preventDefault is evaluated before HandleKeyDown runs, so toggling
shouldPreventDefault inside the handler won’t prevent the newline on Enter. Use a
DOM onkeydown guard to preventDefault() for Enter (without Shift) while keeping the
Blazor handler to send the message, and remove the now-ineffective state flag.

JobScraper.Web/Features/Cv/Components/AiCvChat.razor [20-70]

 <textarea class="form-control"
           rows="4"
           placeholder="Ask about CV or request changes..."
           @bind="userMessage"
           @bind:event="oninput"
           @onkeydown="HandleKeyDown"
-          @onkeydown:preventDefault="shouldPreventDefault"
+          onkeydown="if (event.key === 'Enter' && !event.shiftKey) event.preventDefault();"
           disabled="@IsWorking"/>
 ...
 @code {
     private string userMessage = "";
-    private bool shouldPreventDefault;
     ...
     private async Task HandleKeyDown(KeyboardEventArgs e)
     {
         if (e is { Key: "Enter", ShiftKey: false })
-        {
-            shouldPreventDefault = true;
             await SendMessage();
-        }
-        else
-        {
-            shouldPreventDefault = false;
-        }
     }
     ...
 }
Suggestion importance[1-10]: 7

__

Why: In Blazor, @onkeydown:preventDefault cannot be toggled “within” the same key event (it’s decided before/at dispatch), so updating shouldPreventDefault inside HandleKeyDown won’t reliably stop the newline. Adding a synchronous DOM preventDefault() guard for Enter (without Shift) is a practical way to ensure the textarea doesn’t insert a newline while still allowing HandleKeyDown to call SendMessage().

Medium
Prevent navigation when disabled

Wrapping a inside an is invalid HTML and causes the link to remain clickable even
when the inner button is disabled (so navigation can still happen while isWorking).
Replace these with a single styled and use @onclick:preventDefault="isWorking"
(plus aria-disabled) to truly block navigation when disabled.

JobScraper.Web/Features/Cv/PrepareCvWithAi.razor [45-100]

-<a href="/cv/edit/@cvEntity.OriginCv.Id">
-    <button class="btn btn-info btn-sm"
-            disabled="@isWorking">
-        Origin CV
-    </button>
+<a href="/cv/edit/@cvEntity.OriginCv.Id"
+   class="btn btn-info btn-sm"
+   aria-disabled="@isWorking"
+   @onclick:preventDefault="isWorking">
+    Origin CV
 </a>
 ...
-<a href="/cv/@cvEntity.Id/pdf" target="_blank">
-    <button class="btn btn-primary btn-sm"
-            disabled="@isWorking">
-        Show PDF
-    </button>
+<a href="/cv/@cvEntity.Id/pdf"
+   target="_blank"
+   class="btn btn-primary btn-sm"
+   aria-disabled="@isWorking"
+   @onclick:preventDefault="isWorking">
+    Show PDF
 </a>
 ...
 @if (offer is not null)
 {
-    <a class="ms-1" href="/?offerUrl=@Uri.EscapeDataString(offer.OfferUrl)">
-        <button class="btn btn-info btn-sm" disabled="@isWorking">Offer</button>
+    <a class="btn btn-info btn-sm ms-1"
+       href="/?offerUrl=@Uri.EscapeDataString(offer.OfferUrl)"
+       aria-disabled="@isWorking"
+       @onclick:preventDefault="isWorking">
+        Offer
     </a>
 }
Suggestion importance[1-10]: 6

__

Why: Nesting a <button> inside an <a> is invalid HTML and the link can still be navigated even if the inner button is disabled, which undermines the isWorking guard. Converting these to a single styled <a> and preventing default navigation when isWorking improves correctness and UX, though it won’t block non-click navigations (e.g., “open in new tab”).

Low
Avoid null dereference in comparison

This dereferences cvEntity.MarkdownContent even though other code treats cvEntity as
nullable, which can throw at runtime. Compare against a null-safe saved value and
use StringComparison.Ordinal to avoid unexpected results from culture-sensitive
comparisons.

JobScraper.Web/Features/Cv/CvEditPage.razor.cs [280-297]

 private async Task OnEditorContentChanged()
 {
     if (diffEditor is null)
         return;
 
     var currentContent = await diffEditor.GetModifiedValueAsync();
-    _hasEditorChanges = currentContent != cvEntity.MarkdownContent;
+    var savedContent = cvEntity?.MarkdownContent ?? string.Empty;
+    _hasEditorChanges = !string.Equals(currentContent, savedContent, StringComparison.Ordinal);
 }
 
 private async Task<bool> HasUnsavedEditorChanges()
 {
     if (diffEditor is null)
         return false;
 
     var currentContent = await diffEditor.GetModifiedValueAsync();
-    _hasEditorChanges = currentContent != cvEntity.MarkdownContent;
+    var savedContent = cvEntity?.MarkdownContent ?? string.Empty;
+    _hasEditorChanges = !string.Equals(currentContent, savedContent, StringComparison.Ordinal);
     return _hasEditorChanges;
 }
Suggestion importance[1-10]: 4

__

Why: The current code dereferences cvEntity.MarkdownContent even though cvEntity is treated as nullable elsewhere, so this could throw if the page lifecycle ever calls the handler before initialization. Using cvEntity?.MarkdownContent ?? string.Empty and string.Equals(..., StringComparison.Ordinal) is a safe, low-risk robustness improvement.

Low

@wojciech-dron
wojciech-dron merged commit b52eb98 into master Mar 9, 2026
1 check passed
@wojciech-dron
wojciech-dron deleted the ux-improvements branch March 9, 2026 17:20
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