feat(desktop): Session Profile sharing in Settings → Feedback#1941
Open
richiemcilroy wants to merge 6 commits into
Open
feat(desktop): Session Profile sharing in Settings → Feedback#1941richiemcilroy wants to merge 6 commits into
richiemcilroy wants to merge 6 commits into
Conversation
Co-authored-by: Richie McIlroy <richiemcilroy@users.noreply.github.com>
…S3 upload Co-authored-by: Richie McIlroy <richiemcilroy@users.noreply.github.com>
Co-authored-by: Richie McIlroy <richiemcilroy@users.noreply.github.com>
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| app: AppHandle, | ||
| note: Option<String>, | ||
| ) -> Result<SessionProfileUploadResult, String> { | ||
| let note = note.and_then(|value| { |
There was a problem hiding this comment.
Looks like /api/desktop/session-profile/notify enforces a 4000-char max for note. If the user pastes something long, this will fail after the bundle is uploaded.
Suggested change
| let note = note.and_then(|value| { | |
| let note = note.and_then(|value| { | |
| let trimmed = value.trim(); | |
| if trimmed.is_empty() { | |
| None | |
| } else { | |
| let mut note = trimmed.to_string(); | |
| note.truncate(4000); | |
| Some(note) | |
| } | |
| }); |
| </div> | ||
| </div> | ||
|
|
||
| <textarea |
There was a problem hiding this comment.
Since the server caps note at 4000 chars, it’d be nice to enforce the same in the UI to avoid a late notify failure.
Suggested change
| <textarea | |
| <textarea | |
| value={profileNote()} | |
| onInput={(e) => setProfileNote(e.currentTarget.value)} | |
| disabled={sendingProfile()} | |
| maxLength={4000} | |
| placeholder="Optional: describe the bug or what you were doing when it happened..." | |
| class="p-2 w-full h-24 text-[13px] rounded-md border transition-colors duration-200 resize-none bg-gray-2 placeholder:text-gray-10 border-gray-3 text-primary focus:outline-hidden focus:ring-1 focus:ring-gray-8 hover:border-gray-6 disabled:opacity-50" | |
| /> |
Comment on lines
+622
to
+629
| if (!response.ok) { | ||
| throw new Error( | ||
| `Failed to send session profile to Discord: ${response.statusText}`, | ||
| ); | ||
| } | ||
|
|
||
| discordDelivered = true; | ||
| } |
There was a problem hiding this comment.
If Discord delivery fails, throwing here makes the whole notify call a 500 even though the bundle is uploaded and the downloadUrl is valid. Consider treating Discord as best-effort so the client doesn’t retry/re-upload.
Suggested change
| if (!response.ok) { | |
| throw new Error( | |
| `Failed to send session profile to Discord: ${response.statusText}`, | |
| ); | |
| } | |
| discordDelivered = true; | |
| } | |
| if (!response.ok) { | |
| console.error( | |
| "Failed to send session profile to Discord:", | |
| response.status, | |
| await response.text(), | |
| ); | |
| } else { | |
| discordDelivered = true; | |
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+114
to
+143
| const handleSendProfile = async () => { | ||
| setSendingProfile(true); | ||
| setProfileResult(null); | ||
| setProfileProgress(null); | ||
|
|
||
| let unlisten: (() => void) | undefined; | ||
| try { | ||
| unlisten = await events.sessionProfileProgress.listen((event) => | ||
| setProfileProgress(event.payload), | ||
| ); | ||
| const note = profileNote().trim(); | ||
| const result = await commands.uploadSessionProfile(note || null); | ||
| setProfileResult(result); | ||
| setProfileNote(""); | ||
| toast.success("Session profile sent. Thank you!"); | ||
| } catch (error) { | ||
| console.error("Failed to send session profile:", error); | ||
| toast.error( | ||
| typeof error === "string" | ||
| ? error | ||
| : "Failed to send session profile. Please try again.", | ||
| ); | ||
| } finally { | ||
| unlisten?.(); | ||
| setProfileProgress(null); | ||
| setSendingProfile(false); | ||
| } | ||
| }; | ||
|
|
||
| onCleanup(() => setProfileProgress(null)); |
Contributor
There was a problem hiding this comment.
unlisten function is scoped inside handleSendProfile, so onCleanup cannot reach it. If the user navigates away from the Settings page while an upload is in progress, the sessionProfileProgress Tauri listener keeps firing and calls setProfileProgress on a disposed reactive root until the upload finally finishes.
Suggested change
| const handleSendProfile = async () => { | |
| setSendingProfile(true); | |
| setProfileResult(null); | |
| setProfileProgress(null); | |
| let unlisten: (() => void) | undefined; | |
| try { | |
| unlisten = await events.sessionProfileProgress.listen((event) => | |
| setProfileProgress(event.payload), | |
| ); | |
| const note = profileNote().trim(); | |
| const result = await commands.uploadSessionProfile(note || null); | |
| setProfileResult(result); | |
| setProfileNote(""); | |
| toast.success("Session profile sent. Thank you!"); | |
| } catch (error) { | |
| console.error("Failed to send session profile:", error); | |
| toast.error( | |
| typeof error === "string" | |
| ? error | |
| : "Failed to send session profile. Please try again.", | |
| ); | |
| } finally { | |
| unlisten?.(); | |
| setProfileProgress(null); | |
| setSendingProfile(false); | |
| } | |
| }; | |
| onCleanup(() => setProfileProgress(null)); | |
| let currentUnlisten: (() => void) | undefined; | |
| onCleanup(() => { | |
| currentUnlisten?.(); | |
| currentUnlisten = undefined; | |
| setProfileProgress(null); | |
| }); | |
| const handleSendProfile = async () => { | |
| setSendingProfile(true); | |
| setProfileResult(null); | |
| setProfileProgress(null); | |
| try { | |
| currentUnlisten = await events.sessionProfileProgress.listen((event) => | |
| setProfileProgress(event.payload), | |
| ); | |
| const note = profileNote().trim(); | |
| const result = await commands.uploadSessionProfile(note || null); | |
| setProfileResult(result); | |
| setProfileNote(""); | |
| toast.success("Session profile sent. Thank you!"); | |
| } catch (error) { | |
| console.error("Failed to send session profile:", error); | |
| toast.error( | |
| typeof error === "string" | |
| ? error | |
| : "Failed to send session profile. Please try again.", | |
| ); | |
| } finally { | |
| currentUnlisten?.(); | |
| currentUnlisten = undefined; | |
| setProfileProgress(null); | |
| setSendingProfile(false); | |
| } | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/settings/feedback.tsx
Line: 114-143
Comment:
Event listener not cleaned up on component unmount. The `unlisten` function is scoped inside `handleSendProfile`, so `onCleanup` cannot reach it. If the user navigates away from the Settings page while an upload is in progress, the `sessionProfileProgress` Tauri listener keeps firing and calls `setProfileProgress` on a disposed reactive root until the upload finally finishes.
```suggestion
let currentUnlisten: (() => void) | undefined;
onCleanup(() => {
currentUnlisten?.();
currentUnlisten = undefined;
setProfileProgress(null);
});
const handleSendProfile = async () => {
setSendingProfile(true);
setProfileResult(null);
setProfileProgress(null);
try {
currentUnlisten = await events.sessionProfileProgress.listen((event) =>
setProfileProgress(event.payload),
);
const note = profileNote().trim();
const result = await commands.uploadSessionProfile(note || null);
setProfileResult(result);
setProfileNote("");
toast.success("Session profile sent. Thank you!");
} catch (error) {
console.error("Failed to send session profile:", error);
toast.error(
typeof error === "string"
? error
: "Failed to send session profile. Please try again.",
);
} finally {
currentUnlisten?.();
currentUnlisten = undefined;
setProfileProgress(null);
setSendingProfile(false);
}
};
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+439
to
+447
| let response = request | ||
| .send() | ||
| .await | ||
| .map_err(|err| format!("Failed to request upload URL: {err}"))?; | ||
|
|
||
| if !response.status().is_success() { | ||
| let status = response.status(); | ||
| let body = response.text().await.unwrap_or_default(); | ||
| return Err(format!("Upload URL request failed ({status}): {body}")); |
Contributor
There was a problem hiding this comment.
TempFileGuard::drop: the file could be removed between the exists() check and remove_file(), causing a spurious warning. The idiomatic Rust pattern is to attempt the removal and ignore NotFound — this is both race-free and simpler.
Suggested change
| let response = request | |
| .send() | |
| .await | |
| .map_err(|err| format!("Failed to request upload URL: {err}"))?; | |
| if !response.status().is_success() { | |
| let status = response.status(); | |
| let body = response.text().await.unwrap_or_default(); | |
| return Err(format!("Upload URL request failed ({status}): {body}")); | |
| impl Drop for TempFileGuard { | |
| fn drop(&mut self) { | |
| if let Err(err) = std::fs::remove_file(&self.0) { | |
| if err.kind() != std::io::ErrorKind::NotFound { | |
| warn!(error = %err, path = %self.0.display(), "Failed to clean up session profile bundle"); | |
| } | |
| } | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/session_profile.rs
Line: 439-447
Comment:
TOCTOU race in `TempFileGuard::drop`: the file could be removed between the `exists()` check and `remove_file()`, causing a spurious warning. The idiomatic Rust pattern is to attempt the removal and ignore `NotFound` — this is both race-free and simpler.
```suggestion
impl Drop for TempFileGuard {
fn drop(&mut self) {
if let Err(err) = std::fs::remove_file(&self.0) {
if err.kind() != std::io::ErrorKind::NotFound {
warn!(error = %err, path = %self.0.display(), "Failed to clean up session profile bundle");
}
}
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+605
to
+699
| let mut recordings: Vec<SessionProfileRecording> = Vec::new(); | ||
| if let Some(studio) = status.studio { | ||
| recordings.push(studio); | ||
| } | ||
| if let Some(instant) = status.instant { | ||
| recordings.push(instant); | ||
| } | ||
|
|
||
| if recordings.is_empty() { | ||
| return Err("No Studio or Instant recordings found to profile.".to_string()); | ||
| } | ||
|
|
||
| let is_recording = { | ||
| let app_lock = app.state::<crate::ArcLock<crate::App>>(); | ||
| let state = app_lock.read().await; | ||
| matches!( | ||
| state.recording_state, | ||
| crate::RecordingState::Active(_) | crate::RecordingState::Pending { .. } | ||
| ) | ||
| }; | ||
|
|
||
| let diagnostics = | ||
| logging::collect_diagnostics_for_upload(&recordings_dir, &app_data_dir, is_recording); | ||
| let diagnostics_json = | ||
| serde_json::to_string_pretty(&diagnostics).unwrap_or_else(|_| "{}".to_string()); | ||
| let diagnostics_summary = logging::summarize_diagnostics(&diagnostics); | ||
|
|
||
| let profile_summary = build_profile_summary(&recordings, note.as_deref()); | ||
| let profile_json = | ||
| serde_json::to_string_pretty(&profile_summary).unwrap_or_else(|_| "{}".to_string()); | ||
|
|
||
| let log_file = logging::get_latest_log_file(app).await; | ||
|
|
||
| let file_name = format!( | ||
| "cap-session-profile-{}.zip", | ||
| chrono::Utc::now().format("%Y%m%d-%H%M%S") | ||
| ); | ||
| let zip_path = std::env::temp_dir().join(format!("{}-{file_name}", uuid::Uuid::new_v4())); | ||
| let bundle_guard = TempFileGuard(zip_path.clone()); | ||
|
|
||
| emit_progress( | ||
| app, | ||
| SessionProfileStage::Compressing, | ||
| 0.0, | ||
| "Compressing recordings…", | ||
| ); | ||
|
|
||
| let bundle_size = { | ||
| let app_progress = app.clone(); | ||
| let zip_path = zip_path.clone(); | ||
| let recordings = recordings.clone(); | ||
| let diagnostics_json = diagnostics_json.clone(); | ||
| let profile_json = profile_json.clone(); | ||
| let log_file = log_file.clone(); | ||
|
|
||
| tokio::task::spawn_blocking(move || { | ||
| let mut last_pct: i64 = -1; | ||
| build_profile_zip( | ||
| &zip_path, | ||
| &recordings, | ||
| &diagnostics_json, | ||
| &profile_json, | ||
| log_file.as_deref(), | ||
| |done, total| { | ||
| let pct = if total > 0 { | ||
| ((done.saturating_mul(100)) / total) as i64 | ||
| } else { | ||
| 100 | ||
| }; | ||
| if pct != last_pct { | ||
| last_pct = pct; | ||
| emit_progress( | ||
| &app_progress, | ||
| SessionProfileStage::Compressing, | ||
| pct as f64 / 100.0, | ||
| "Compressing recordings…", | ||
| ); | ||
| } | ||
| }, | ||
| ) | ||
| }) | ||
| .await | ||
| .map_err(|err| format!("Bundle task failed: {err}"))?? | ||
| }; | ||
|
|
||
| let client = app | ||
| .state::<RetryableHttpClient>() | ||
| .as_ref() | ||
| .map_err(|err| format!("HTTP client unavailable: {err:?}"))? | ||
| .clone(); | ||
| let base_url = app.make_app_url("").await; | ||
| let bearer = auth_bearer_token(app)?; | ||
|
|
||
| emit_progress( | ||
| app, |
Contributor
There was a problem hiding this comment.
build_profile_zip streams entire recording directories into a temp file without any upper-bound check. A user with several large Studio recordings (each can easily be multiple GB) could fill /tmp and then attempt a multi-GB streaming PUT to the presigned S3 URL. There is no feedback to the user about estimated size before the operation starts, and no hard cap to protect disk space. Consider computing the total uncompressed size up-front (you already have recording.size_bytes) and either warning the UI or refusing uploads above a reasonable limit (e.g. 5–10 GB).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/session_profile.rs
Line: 605-699
Comment:
**No bundle size guard before zipping or uploading.** `build_profile_zip` streams entire recording directories into a temp file without any upper-bound check. A user with several large Studio recordings (each can easily be multiple GB) could fill `/tmp` and then attempt a multi-GB streaming PUT to the presigned S3 URL. There is no feedback to the user about estimated size before the operation starts, and no hard cap to protect disk space. Consider computing the total uncompressed size up-front (you already have `recording.size_bytes`) and either warning the UI or refusing uploads above a reasonable limit (e.g. 5–10 GB).
How can I resolve this? If you propose a fix, please make it concise.…nd cap note length Co-authored-by: Richie McIlroy <richiemcilroy@users.noreply.github.com>
…n temp cleanup Co-authored-by: Richie McIlroy <richiemcilroy@users.noreply.github.com>
Co-authored-by: Richie McIlroy <richiemcilroy@users.noreply.github.com>
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| .and_then(|name| name.to_str()) | ||
| .unwrap_or("recording"); | ||
|
|
||
| for entry in WalkDir::new(root).into_iter().filter_map(Result::ok) { |
There was a problem hiding this comment.
P2: WalkDir follows symlinks, allowing arbitrary files outside recordings to be bundled
WalkDir follows symlinks by default during bundle creation, allowing files outside the recording directory to be included in the uploaded zip.
Call .follow_links(false) on WalkDir instances to prevent symlink traversal during profiling.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="apps/desktop/src-tauri/src/session_profile.rs">
<violation number="1" location="apps/desktop/src-tauri/src/session_profile.rs:394">
<priority>P2</priority>
<title>WalkDir follows symlinks, allowing arbitrary files outside recordings to be bundled</title>
<evidence>The build_profile_zip function uses WalkDir::new(root) without disabling symlink following. By default WalkDir follows symlinks, so a symlink placed inside a recording directory pointing to sensitive files outside the directory (e.g., /etc/passwd) would be opened and bundled into the zip, then uploaded to S3.</evidence>
<recommendation>Set .follow_links(false) on WalkDir instances in build_profile_zip (and directory_size for consistency) to prevent traversal of symlinks when discovering recording contents.</recommendation>
</violation>
</file>
|
|
||
| let mut done: u64 = 0; | ||
|
|
||
| let file = std::fs::File::create(output_path) |
There was a problem hiding this comment.
P2: Session profile temp bundle may be created with world-readable permissions
build_profile_zip creates the bundle in the system temp directory with default filesystem permissions, which may allow other local users to read sensitive user data during bundling.
Restrict the temp file permissions to owner-only immediately after creation, or use a secure temp directory.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="apps/desktop/src-tauri/src/session_profile.rs">
<violation number="1" location="apps/desktop/src-tauri/src/session_profile.rs:360">
<priority>P2</priority>
<title>Session profile temp bundle may be created with world-readable permissions</title>
<evidence>build_profile_zip calls std::fs::File::create(output_path) where output_path is a temp directory path. On Unix systems with a typical umask of 022, this creates the file with 644 permissions, making it world-readable. The bundle contains diagnostics, logs, and potentially recording data that should remain private to the user.</evidence>
<recommendation>Set restrictive file permissions (e.g., 0o600) on the bundle file immediately after creation. On Unix, use std::os::unix::fs::PermissionsExt, and consider applying the same for intermediate files.</recommendation>
</violation>
</file>
superagent-security
Bot
added
the
pr:flagged
Member
Author
|
hey @greptileai please re-review the pr |
Comment on lines
+596
to
+601
| note ? `**Note:** ${note}` : null, | ||
| recordingLines.length > 0 ? "" : null, | ||
| recordingLines.length > 0 ? "**Recordings:**" : null, | ||
| ...recordingLines, | ||
| "", | ||
| `**Download (expires in 7 days):** ${downloadUrl}`, |
Contributor
There was a problem hiding this comment.
note appears before the download link in the message array. A note longer than ~1400 characters — well within the 4000-char schema and UI limit — pushes the download URL past Discord's 2000-character hard cut-off. Since the presigned URL is the only way for the team to retrieve the bundle (it is never displayed in the desktop UI), any profile submitted with a moderately detailed note becomes inaccessible. Moving the **Download …** line to appear before the note block guarantees it is always included.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/desktop/[...route]/root.ts
Line: 596-601
Comment:
**Download URL disappears from Discord when note is long.** `note` appears before the download link in the message array. A note longer than ~1400 characters — well within the 4000-char schema and UI limit — pushes the download URL past Discord's 2000-character hard cut-off. Since the presigned URL is the only way for the team to retrieve the bundle (it is never displayed in the desktop UI), any profile submitted with a moderately detailed note becomes inaccessible. Moving the `**Download …**` line to appear before the note block guarantees it is always included.
How can I resolve this? If you propose a fix, please make it concise.
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
2 participants
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.
Overview
Adds a Session Profiling system to Settings → Feedback so a user can hand us everything we need to fully understand, reproduce and fix a bug from their session in one click.
When triggered, the desktop app:
diagnostics.json), a profile summary (profile.json), and the latest log file into a single zip.Behavior per the request:
Walkthrough
Populated state (latest Studio + Instant recordings listed, with sizes/dates, optional note, and Send button):
Empty state (prompts the user to record first):
Rust test results (incl. end-to-end upload→S3→notify round-trip against a mock server):
session_profile_tests.log
Changes
Desktop (Rust) —
apps/desktop/src-tauri/src/session_profile.rs(new)find_latest_recordingsdiscovers the newest Studio/Instant.capbundles.build_profile_zipstreams each recording dir + diagnostics + summary + log into a zip (stored for media, deflate for text; zip64 for large files).SessionProfileProgressevent for live UI progress.get_session_profile_status,upload_session_profile.logging.rsexposescollect_diagnostics_for_upload+ a newsummarize_diagnostics(Markdown summary for Discord) +get_latest_log_file.web_api.rsexposesapply_env_headers.Web —
apps/web/app/api/desktop/[...route]/root.tsPOST /api/desktop/session-profile/create→ presigned S3 PUT URL scoped todesktop-session-profiles/{userId}/{id}/.POST /api/desktop/session-profile/notify→ validates the key belongs to the user, generates a 7‑day presigned download URL, and posts the link + diagnostics summary toDISCORD_FEEDBACK_WEBHOOK_URL.Desktop (UI) —
apps/desktop/src/routes/(window-chrome)/settings/feedback.tsxGenerated
tauri.tsbindings updated for the new commands/event/types.Testing
cargo test -p cap-desktop session_profile— 5 tests pass, includingupload_and_notify_round_tripwhich builds a real zip and exercises create → S3 PUT → notify against a local mock server.cargo build -p cap-desktop,cargo fmt --check,cargo clippy -p cap-desktopall clean.Note: the full upload→S3→Discord path can't be exercised from the GUI in this environment (requires sign-in + live S3 + Discord webhook), so it is covered by the end-to-end Rust round-trip test instead.
Greptile Summary
This PR adds a Session Profile sharing feature to Settings → Feedback: the desktop app bundles the user's latest Studio and Instant recordings with system diagnostics and logs into a zip, uploads it to S3 via a presigned URL, and posts a Discord notification with a download link.
session_profile.rs): discovers recordings, compresses them with correct zip64 support and media/text compression selection, enforces a 10 GB size guard, and streams the upload with live progress events.root.ts): two new endpoints scope S3 keys to the authenticated user and post a Discord notification; key ownership is validated before generating a download URL.feedback.tsx): renders available recordings, an optional note textarea, and a progress bar; the Tauri event listener is correctly cleaned up viaonCleanup.Confidence Score: 4/5
Safe to merge after fixing the Discord message ordering in root.ts; all other paths are sound.
The notify endpoint places the user note before the download URL in the Discord message. A note exceeding ~1400 characters pushes the presigned download link past Discord's 2000-character cut-off, making the uploaded bundle unreachable by the team. Everything else — S3 key scoping, zip64 handling, size guard, temp-file cleanup, and frontend event-listener teardown — looks correct.
apps/web/app/api/desktop/[...route]/root.ts — the Discord message ordering around line 596 needs the download URL moved above the note.
Important Files Changed
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(web): treat session profile Discord ..." | Re-trigger Greptile