Skip to content

Cv edit and prepare for offer pages - #9

Closed
wojciech-dron wants to merge 6 commits into
masterfrom
cv-modify
Closed

wojciech-dron wants to merge 6 commits into
masterfrom
cv-modify

Conversation

@wojciech-dron

Copy link
Copy Markdown
Owner

No description provided.

@github-actions

github-actions Bot commented Mar 3, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Retry Logic

The retry loop recreates a new AgentGroupChat each attempt and then determines success based on the last streamed response.Content. Depending on agent ordering/streaming, the “last message” may not be the final CV editor output (or may not include the done marker), causing false failures or retries. Consider ensuring termination/success checks are tied to the intended final agent and/or explicitly selecting the final message from CvEditor before validating DoneSignal.

public async ValueTask<Response> Handle(Request request, CancellationToken cancellationToken)
{
    _logger.LogInformation("Adjusting CV content to offer");

    var offerMessage = new ChatMessageContent(AuthorRole.User, $"offerContent: {request.OfferContent}");

    string? finalContent = null;
    var retryCount = 0;
    var chatHistory = new List<ChatItem>();
    do
    {
        var chat = PrepareAgentsChat(request);
        chat.AddChatMessage(offerMessage);

        if (request.OfferSummary is not null)
        {
            var summaryMessage = new ChatMessageContent(AuthorRole.User, $"offerSummary: {request.OfferSummary}");
            chat.AddChatMessage(summaryMessage);
        }

        var asyncResponse = chat.InvokeAsync(cancellationToken);

        await foreach (var response in asyncResponse)
        {
            chatHistory.Add(ChatItem.From(response));
            finalContent = response.Content;
        }
    } while (ShouldRetry(finalContent, retryCount++));

    if (finalContent is null || !finalContent.Contains(DoneSignal))
        return new Response(false, "", chatHistory);

    var adjustedCv = finalContent.Replace(DoneSignal, "").Trim();

    return new Response(true, adjustedCv, chatHistory);
}
State/UI Bugs

There are a few places where UI state can break: removing the last chat item assumes chatHistory is non-empty, and updating the editor model is triggered without awaiting, which can lead to race conditions and missed re-renders. Consider guarding list operations and awaiting editor updates (and calling StateHasChanged when needed) to keep the modal consistent.

    var result = await MessageBus.InvokeAsync<AdjustCvContentToOffer.Response>(request);

    isWorking = false;
    chatHistory = result.ChatHistory;

    if (!result.Success)
    {
        toasts.PushMessage("Failed to prepare CV content. Check agents chat history.", ToastType.Warning);
        return;
    }

    toasts.PushMessage("CV content prepared successfully");

    chatHistory.RemoveAt(chatHistory.Count - 1); // remove last item because it is CV content
    ModifiedContent = result.AdjustedCvContent;
    _ =_diffEditor.SetModifiedModel(ModifiedContent);
}
File IO

TestData.Image eagerly reads a relative-path file at type initialization time. This can fail in production/test environments where the working directory differs, and it introduces blocking IO during startup. Prefer lazy loading and/or resolving the path via app base directory/content root (or embedding as a resource) and handling missing-file scenarios gracefully.

public static class TestData
{
    public static readonly byte[] Image = File.ReadAllBytes("Features/Cv/Components/image.jpg");

@github-actions

github-actions Bot commented Mar 3, 2026

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent startup crashes from file IO

Avoid reading a file in a static field initializer, because it can throw during type
initialization and crash the app at startup when the file/path is missing in non-dev
environments. Load the image lazily and return an empty array (or a safe fallback)
when the file is unavailable, using a deterministic base path.

JobScraper.Web/Features/Cv/Components/TestData.cs [7-9]

 public static class TestData
 {
-    public static readonly byte[] Image = File.ReadAllBytes("Features/Cv/Components/image.jpg");
+    private static byte[]? _image;
+    public static byte[] Image
+    {
+        get
+        {
+            if (_image is not null) return _image;
 
+            var path = Path.Combine(AppContext.BaseDirectory, "Features", "Cv", "Components", "image.jpg");
+            _image = File.Exists(path) ? File.ReadAllBytes(path) : [];
+            return _image;
+        }
+    }
+
Suggestion importance[1-10]: 7

__

Why: Reading a file in a static field initializer (TestData.Image) can throw during type initialization and fail the app/component startup if the file isn't present in the output. Lazy-loading with a safe fallback improves runtime robustness, especially since the path is environment-dependent.

Medium
Fix unsafe list access and races

Guard against empty chatHistory before calling RemoveAt, since an empty/partial
agent response can still produce a successful result depending on provider behavior.
Also await _diffEditor.SetModifiedModel(...) to avoid race conditions where the UI
does not reflect the updated markdown (and to surface any JS interop errors).

JobScraper.Web/Features/Cv/PrepareCvForOfferModal.razor [158-160]

-chatHistory.RemoveAt(chatHistory.Count - 1); // remove last item because it is CV content
+if (chatHistory.Count > 0)
+    chatHistory.RemoveAt(chatHistory.Count - 1); // remove last item because it is CV content
+
 ModifiedContent = result.AdjustedCvContent;
-_ =_diffEditor.SetModifiedModel(ModifiedContent);
 
+await _diffEditor.SetModifiedModel(ModifiedContent);
+
Suggestion importance[1-10]: 6

__

Why: chatHistory.RemoveAt(chatHistory.Count - 1) can throw if the list is unexpectedly empty, and not awaiting _diffEditor.SetModifiedModel(...) can hide JS/interop failures and lead to UI update timing issues. Adding a count guard and await improves stability with minimal change.

Low
Make done-signal handling robust

Make the completion-signal detection consistent with the termination strategy by
using a case-insensitive check. Also remove the done marker using the same
comparison to avoid returning CV content that still contains the marker when casing
differs.

JobScraper.Web/Features/AiSummary/Logic/AdjustCvContentToOffer.cs [70-73]

-if (finalContent is null || !finalContent.Contains(DoneSignal))
+if (finalContent is null || finalContent.IndexOf(DoneSignal, StringComparison.OrdinalIgnoreCase) < 0)
     return new Response(false, "", chatHistory);
 
-var adjustedCv = finalContent.Replace(DoneSignal, "").Trim();
+var adjustedCv = finalContent.Replace(DoneSignal, "", StringComparison.OrdinalIgnoreCase).Trim();
Suggestion importance[1-10]: 5

__

Why: Completion detection currently uses case-sensitive Contains, while ApprovalTerminationStrategy checks DoneSignal case-insensitively; aligning these avoids false negatives when providers vary casing. Using a case-insensitive removal also prevents returning content that still includes the marker.

Low

@wojciech-dron
wojciech-dron deleted the cv-modify 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