Skip to content

Mediator restore - #12

Closed
wojciech-dron wants to merge 5 commits into
masterfrom
mediator-restore
Closed

wojciech-dron wants to merge 5 commits into
masterfrom
mediator-restore

Conversation

@wojciech-dron

Copy link
Copy Markdown
Owner

No description provided.

@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Possible Issue

ReplaceText always performs replacements against OriginalText and then assigns to ModifiedText, which means multiple tool calls will not compose (a second replacement can overwrite/lose changes from the first one). Consider applying replacements on ModifiedText (current state) to support incremental edits as intended by the agent instructions.

[KernelFunction("replace_text")]
[Description("Replaces a chunk of text of the provided content with new text." +
    " Returns information if the text was not found or if the replacement was successful.")]
public string ReplaceText(
    [Description("The exact text chunk to find and replace")]
    string oldText,
    [Description("The new text to replace it with")]
    string newText)
{
    if (string.IsNullOrEmpty(OriginalText) || string.IsNullOrEmpty(oldText))
        return "No text to replace.";

    if (!OriginalText.Contains(oldText))
        return "[WARN] The specified text was not found in the content. No changes made.";

    ModifiedText = OriginalText.Replace(oldText, newText);

    return "Success";
UI Stuck

The modal sets isWorking = true before sending the request, but the new flow doesn’t set isWorking = false after Mediator.Send(...) completes (both for success and error paths). This can leave the modal permanently disabled/spinning.

StateHasChanged();

var message = new DuplicateCv.Request(OriginCvId, NewCvName, TargetOfferUrl);
var response = await Mediator.Send(message);

if (response.IsError)
{
Navigation Bug

After deleting the CV, navigation uses /?offerUrl={offer} which likely interpolates the UserOffer object rather than a stable identifier (e.g., offer.OfferUrl). This can produce an invalid URL and break redirect behavior. Also consider disposing the imported JS module to avoid leaking JS object references.

private async Task DeleteCvAsync()
{
    var confirmation = await dialog.ShowAsync(
        title: "Are you sure you want to delete this?",
        message1: "This will delete the record. Once deleted can not be rolled back.",
        message2: "Do you want to proceed?");

    if (!confirmation)
        return;

    var cv = await dbContext.Cvs.FindAsync(cvEntity.Id);
    if (cv is null)
        return;

    dbContext.Remove(cv);
    await dbContext.SaveChangesAsync();
    navigationManager.NavigateTo($"/?offerUrl={offer}");
}
private void GoToCv(long duplicatedCvId) => navigationManager.NavigateTo($"cv/edit/{duplicatedCvId}", true);

private enum CompareMode
{
    None,
    WithSaved,
    WithOrigin,
}

public bool DisableCompareToOrigin =>
    compareMode is CompareMode.WithOrigin || string.IsNullOrWhiteSpace(cvEntity?.OriginCv?.MarkdownContent);
private async Task CompareToOrigin()
{
    ArgumentException.ThrowIfNullOrWhiteSpace(cvEntity?.OriginCv?.MarkdownContent);
    compareMode = CompareMode.WithOrigin;
    originalContent = cvEntity?.OriginCv?.MarkdownContent!;
    await diffEditor!.SetOriginalModel(cvEntity!.OriginCv!.MarkdownContent);
}
public bool DisableCompareToSaved => compareMode is CompareMode.WithSaved;
private async Task CompareToSaved()
{
    ArgumentException.ThrowIfNullOrWhiteSpace(cvEntity?.MarkdownContent);
    compareMode = CompareMode.WithSaved;
    originalContent = cvEntity?.MarkdownContent!;
    await diffEditor!.SetOriginalModel(cvEntity!.MarkdownContent);
}
private bool PreventNavigation => isWorking || dbContext.Entry(cvEntity).State == EntityState.Modified;
private async Task OnBeforeInternalNavigation(LocationChangingContext ctx)
{
    if (!PreventNavigation)
        return;

    var confirm = await js.InvokeAsync<bool>("confirm", "Are you sure you want to leave this page?");
    if (!confirm)
        ctx.PreventNavigation();
}

public async ValueTask DisposeAsync()
{
    await _cts.CancelAsync();
    _cts.Dispose();

    if (downloadModule is not null)
        await downloadModule.DisposeAsync();
}

@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Always reset busy UI state

Resetting isWorking was removed after sending the request, which can leave the modal
stuck in a “working” state forever. Wrap the send in try/finally (or set isWorking =
false immediately after) to guarantee UI state is restored even on exceptions.

JobScraper.Web/Features/Cv/Components/DuplicateCvModal.razor [116-124]

 isWorking = true;
 StateHasChanged();
 
 var message = new DuplicateCv.Request(OriginCvId, NewCvName, TargetOfferUrl);
-var response = await Mediator.Send(message);
+ErrorOr<DuplicateCv.Response> response;
+
+try
+{
+    response = await Mediator.Send(message);
+}
+finally
+{
+    isWorking = false;
+    StateHasChanged();
+}
 
 if (response.IsError)
 {
     toasts.PushMessage(response.FirstError.Description, ToastType.Warning);
Suggestion importance[1-10]: 8

__

Why: In DuplicateCvModal.razor, isWorking is set to true and never reset, so the modal can remain stuck in a busy/disabled state. Wrapping Mediator.Send(...) with try/finally (or resetting immediately after) is a correctness/UX fix and prevents permanent lock-up even on exceptions.

Medium
Apply edits cumulatively

ReplaceText currently searches and replaces against OriginalText, so multiple tool
calls will overwrite prior edits and lose changes. Apply replacements to
ModifiedText (and check existence there) so successive edits accumulate correctly.

JobScraper.Web/Features/Cv/Logic/ReplaceTextPlugin.cs [32-40]

-if (string.IsNullOrEmpty(OriginalText) || string.IsNullOrEmpty(oldText))
+if (string.IsNullOrEmpty(ModifiedText) || string.IsNullOrEmpty(oldText))
     return "No text to replace.";
 
-if (!OriginalText.Contains(oldText))
+if (!ModifiedText.Contains(oldText))
     return "[WARN] The specified text was not found in the content. No changes made.";
 
-ModifiedText = OriginalText.Replace(oldText, newText);
+ModifiedText = ModifiedText.Replace(oldText, newText);
 
 return "Success";
Suggestion importance[1-10]: 8

__

Why: ReplaceTextPlugin.ReplaceText currently replaces against OriginalText, so multiple replace_text tool calls will overwrite previous changes instead of accumulating edits. Switching the search/replace base to ModifiedText makes the tool behave correctly for iterative modifications.

Medium
Prevent infinite model loops

This loop can run indefinitely if the model never includes DoneSignal, causing a
hang until external cancellation. Add an explicit iteration limit (and/or check
ct.IsCancellationRequested) and fail gracefully when exceeded.

JobScraper.Web/Features/Cv/Logic/AiCvWithModifyConversation.cs [53-57]

 var lastResponse = "";
-do
+const int maxIterations = 5;
+
+for (var i = 0; i < maxIterations; i++)
+{
     await foreach (var response in agent.InvokeAsync(chatHistory, cancellationToken: ct))
         lastResponse = response.Message.Content ?? "";
-while (!lastResponse.Contains(DoneSignal));
 
+    if (lastResponse.Contains(DoneSignal))
+        break;
+}
+
+if (!lastResponse.Contains(DoneSignal))
+    return new Response(false, null, chatHistory.Select(ChatItem.From).ToList());
+
Suggestion importance[1-10]: 6

__

Why: The do/while loop waits indefinitely for DoneSignal, which can hang if the model never emits it (until external cancellation). Adding a maximum iteration limit (and failing gracefully) improves robustness without changing the intended flow.

Low

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