RemoteEdit Draft Management and Recovery - #241
Open
nschimme wants to merge 7 commits into
Open
Conversation
- Consolidate MPI session and draft management into RemoteEdit - Decouple Proxy from RemoteEdit using signals/slots - Move RemoteEdit ownership to MainWindow - Implement draft file persistence in MMapper/Editor/ - Implement atomic auto-save with debounce (2s) and throttle (15s) - Implement draft recovery scan at startup - Integrate RemoteEdit tasks with TasksPanel and AsyncTask system - Add 'Show Editor' functionality for active and recovered tasks
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideRefactors RemoteEdit into a MainWindow-owned, long-lived draft manager with async task integration, persistent draft files, and startup recovery of orphaned drafts, while decoupling Proxy via signals/slots and wiring UI for auto-save and recovered-draft viewing. Sequence diagram for RemoteEdit draft lifecycle and recoverysequenceDiagram
actor User
participant MF as MpiFilterToMud
participant PX as Proxy
participant MW as MainWindow
participant RE as RemoteEdit
participant SES as RemoteEditSession
participant AT as AsyncTasks
participant TP as TasksPanel
MF->>PX: slot_remoteEdit(id, title, body)
PX->>RE: sig_remoteEditRequested(sessionId, title, body)
RE->>RE: addSession(sessionId, title, body)
RE->>RE: provisionDraftFile(sessionId, title, body)
RE->>SES: setDraftFileName(fileName)
RE->>AT: startAsyncTask(RemoteEdit,...)
AT-->>RE: AsyncTaskHandle
RE->>SES: setAsyncTask(handle)
rect rgb(230,230,255)
User->>TP: click "Show Editor" for RemoteEdit task
TP->>MW: getRemoteEdit()
MW->>RE: raiseSession(taskId)
RE->>SES: getSessionByTaskId(taskId)
SES->>SES: raise()
end
rect rgb(230,255,230)
MW->>RE: recoverDrafts()
RE->>RE: discoverDrafts()
RE->>SES: create recovered RemoteEditSession(...)
RE->>AT: startAsyncTask(RemoteEdit,...)
AT-->>RE: AsyncTaskHandle
RE->>SES: setAsyncTask(handle)
RE->>SES: setDraftFileName(...)
RE->>SES: setDisconnected()
end
rect rgb(255,230,230)
SES->>RE: save()
RE->>PX: sig_remoteEditSave(sessionId, content)
PX->>MF: saveRemoteEdit(sessionId, content)
RE->>RE: deleteDraft(draftFileName)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The async task loop in RemoteEdit::addSession/RemoteEdit::recoverDrafts relies on RemoteEditSession::shouldStopTask without any synchronization, which can lead to data races between the worker thread and the GUI thread; consider using an atomic flag or another thread-safe mechanism for signalling task termination.
- RemoteEditExternalSession currently passes getFullDraftPath() (which is empty at construction time) into RemoteEditProcess, so external edit sessions fall back to a temp file instead of the provisioned draft path; you likely want to provision the draft and set m_draftFileName before constructing RemoteEditExternalSession so the external editor uses the persistent draft file.
- In connectionlistener.h, replacing the forward declaration of Proxy with a local #include "proxy.h" inside the forward-declarations block introduces a tighter header coupling and potential circular-dependency issues; it would be cleaner to restore the forward declaration and move the include to the top of the file only where a full definition is needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The async task loop in RemoteEdit::addSession/RemoteEdit::recoverDrafts relies on RemoteEditSession::shouldStopTask without any synchronization, which can lead to data races between the worker thread and the GUI thread; consider using an atomic flag or another thread-safe mechanism for signalling task termination.
- RemoteEditExternalSession currently passes getFullDraftPath() (which is empty at construction time) into RemoteEditProcess, so external edit sessions fall back to a temp file instead of the provisioned draft path; you likely want to provision the draft and set m_draftFileName before constructing RemoteEditExternalSession so the external editor uses the persistent draft file.
- In connectionlistener.h, replacing the forward declaration of Proxy with a local #include "proxy.h" inside the forward-declarations block introduces a tighter header coupling and potential circular-dependency issues; it would be cleaner to restore the forward declaration and move the include to the top of the file only where a full definition is needed.
## Individual Comments
### Comment 1
<location path="src/mpi/remoteedit.cpp" line_range="74-83" />
<code_context>
+ if (isEdit) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Async task lambda holds a raw session pointer, which can dangle when the session is removed.
In `addSession()`, the RemoteEdit async task captures `auto* pSession = session.get()` by value and uses it in a long-running loop. In `removeSession()`, `stopTask()` is called and the session is erased from `m_sessions`, destroying the `shared_ptr` and the session object while the async thread may still access `pSession->shouldStopTask()`, leading to undefined behavior. The recovered-drafts path correctly captures a `std::shared_ptr` and uses `get()` only inside the lambda. Please change the regular edit path to capture a `std::shared_ptr<RemoteEditSession>` (or otherwise ensure the task owns the session and the session outlives the task).
</issue_to_address>
### Comment 2
<location path="src/mpi/remoteedit.cpp" line_range="346-349" />
<code_context>
+ return false;
+}
+
+void RemoteEdit::deleteDraft(const QString &fileName)
+{
+ if (fileName.isEmpty()) return;
+ QFile::remove(QDir(getDraftDirectory()).absoluteFilePath(fileName));
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** deleteDraft silently ignores failure to remove the draft file.
If the draft directory isn’t writable or the file can’t be removed, `deleteDraft` fails silently. Since drafts drive recovery (`discoverDrafts`), leftover files could cause confusing behaviour (e.g. tasks “recovered” after being cancelled). Please log a warning when `QFile::remove` returns false to aid diagnosing editorDirectory or filesystem permission issues.
Suggested implementation:
```cpp
void RemoteEdit::deleteDraft(const QString &fileName)
{
if (fileName.isEmpty()) {
return;
}
const QString draftPath = QDir(getDraftDirectory()).absoluteFilePath(fileName);
QFile draftFile(draftPath);
if (!draftFile.remove()) {
qWarning() << "RemoteEdit::deleteDraft: failed to remove draft file"
<< draftPath
<< "- error:" << draftFile.errorString();
}
}
```
If `qWarning` or `QFile`/`QDir` are not yet included in this translation unit, you may need to ensure the appropriate Qt headers are present (typically `<QFile>`, `<QDir>`, and, if required by your logging setup, `<QDebug>` or `<QtGlobal>`). Align the logging style with the rest of the file (e.g. if it uses a custom logging helper or `QLoggingCategory`, adapt the `qWarning` call accordingly).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+346
to
+349
| void RemoteEdit::deleteDraft(const QString &fileName) | ||
| { | ||
| if (fileName.isEmpty()) return; | ||
| QFile::remove(QDir(getDraftDirectory()).absoluteFilePath(fileName)); |
There was a problem hiding this comment.
suggestion (bug_risk): deleteDraft silently ignores failure to remove the draft file.
If the draft directory isn’t writable or the file can’t be removed, deleteDraft fails silently. Since drafts drive recovery (discoverDrafts), leftover files could cause confusing behaviour (e.g. tasks “recovered” after being cancelled). Please log a warning when QFile::remove returns false to aid diagnosing editorDirectory or filesystem permission issues.
Suggested implementation:
void RemoteEdit::deleteDraft(const QString &fileName)
{
if (fileName.isEmpty()) {
return;
}
const QString draftPath = QDir(getDraftDirectory()).absoluteFilePath(fileName);
QFile draftFile(draftPath);
if (!draftFile.remove()) {
qWarning() << "RemoteEdit::deleteDraft: failed to remove draft file"
<< draftPath
<< "- error:" << draftFile.errorString();
}
}
If qWarning or QFile/QDir are not yet included in this translation unit, you may need to ensure the appropriate Qt headers are present (typically <QFile>, <QDir>, and, if required by your logging setup, <QDebug> or <QtGlobal>). Align the logging style with the rest of the file (e.g. if it uses a custom logging helper or QLoggingCategory, adapt the qWarning call accordingly).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #241 +/- ##
==========================================
+ Coverage 25.08% 26.07% +0.98%
==========================================
Files 528 528
Lines 44211 45812 +1601
Branches 4793 4847 +54
==========================================
+ Hits 11092 11944 +852
- Misses 33119 33868 +749 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Centralize MPI session and draft management in RemoteEdit - MainWindow now owns the long-lived RemoteEdit instance - Decouple Proxy from RemoteEdit via direct GMCP subscription in RemoteEdit - Mark all active and recovered edits as AsyncTasks for unified management - Implement draft persistence in MMapper/Editor directory - Implement atomic auto-save with debounce (2s) and throttle (15s) - Add draft recovery scan at startup and immediately after disconnects - Integrate 'Show Editor' in TasksPanel to raise windows or open read-only drafts - Ensure drafts are purged only upon confirmed write/cancel from server - Handle clean and unclean disconnects to preserve draft state
- Establish MMapper/Editor/ scratch directory for draft persistence. - Centralize RemoteEdit management in MainWindow and decouple from Proxy via GMCP signals. - Implement atomic auto-save with 2000ms debounce and 15000ms throttle. - Add startup recovery sequence to scan for orphaned draft files. - Ensure thread safety in background tasks using weak_ptr for session access. - Preserve draft files across disconnections until explicit server confirmation. - Update TasksPanel with "Show Editor" functionality for active and recovered drafts. - Enhance UX for external editor tasks with status notifications.
- Add virtual isRunning() to RemoteEditSession base class. - Override isRunning() in RemoteEditExternalSession to check process state. - Use virtual isRunning() in RemoteEdit::raiseSession to avoid conditional compilation issues with external sessions. - Fix variable shadowing in slot_parseGmcpInput by renaming local error message variable. - Ensure consistent behavior across all platforms including Wasm and Snap.
- Establish MMapper/Editor/ scratch directory for draft persistence. - Centralize RemoteEdit management in MainWindow and decouple from Proxy via GMCP signals. - Implement atomic auto-save with 2000ms debounce and 15000ms throttle. - Add startup recovery sequence to scan for orphaned draft files. - Ensure thread safety in background tasks using weak_ptr for session access. - Preserve draft files across disconnections until explicit server confirmation. - Transition disconnected closed sessions to recovered state in the task list. - Simplified UX: Removed 'raise' feature and clipboard copy on disconnect.
- Centralize RemoteEdit management in MainWindow and decouple from Proxy. - Route MUME.Client GMCP messages via Proxy signals for protocol stability. - Implement atomic auto-saving with 2s debounce and 15s throttle using QSaveFile. - Add recovery logic for orphaned drafts on application startup. - Update TasksPanel with a functional UI to view and extract recovered drafts. - Fulfills functional requirements FR-0.1 through FR-6.4.
- Removed stale connection to m_remoteEdit->slot_parseGmcpInput in MainWindow. - Decoupled RemoteEdit result handling from generic GMCP parsing. - Added virtual methods to MudTelnetOutputs for remote write/cancel results. - Proxy now emits specific signals for remote edit results. - RemoteEdit now deletes drafts only upon confirmed server success via slot_remoteWriteResult. - This fixes the CI failure where RemoteEdit had no member slot_parseGmcpInput.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change implements the RemoteEdit Draft Management and Recovery Agent. It refactors the existing RemoteEdit system to centralize draft persistence and session management. MainWindow now owns the long-lived RemoteEdit instance, and Proxy is decoupled via signals/slots. Drafts are automatically saved with debounce and throttle logic, and orphaned drafts are recovered as background tasks upon application startup. recovered drafts can be inspected in a read-only viewer.
PR created automatically by Jules for task 18138830753454044249 started by @nschimme
Summary by Sourcery
Centralize RemoteEdit lifecycle in MainWindow, introduce persistent draft storage with recovery, and integrate edit tasks into the async task and UI systems.
New Features:
Enhancements: