From 1a0c0537ce68fb3505946f81483e2310c98e56f2 Mon Sep 17 00:00:00 2001 From: jaeyunha Date: Tue, 16 Jun 2026 19:17:13 -0700 Subject: [PATCH] wip: actions job-log page UX refinements Backed up before worktree cleanup. QA: biome clean, vitest 8/8 pass (web). Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.toml | 1 + crates/api/Cargo.toml | 1 + crates/api/src/domain/milestones.rs | 10 ++-- crates/api/src/domain/organizations.rs | 4 +- crates/api/src/domain/packages.rs | 14 ++++- crates/api/src/routes/actions.rs | 16 +++++- crates/api/src/routes/repositories.rs | 26 +++------ ...api_repository_actions_job_log_contract.rs | 54 +++++++++++++++++- crates/api/tests/owner_packages_contract.rs | 9 ++- .../api/tests/projects_workspace_contract.rs | 5 +- .../api/tests/repository_network_contract.rs | 7 ++- .../jobs/[jobId]/logs/download/route.ts | 8 ++- .../RepositoryActionsJobLogPage.tsx | 55 ++++++++++++------- .../e2e/repository-actions-job-log.spec.ts | 2 +- web/tests/repository-actions-job-log.test.tsx | 34 ++++++++---- 15 files changed, 173 insertions(+), 73 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bf02bf9c..d6e07f56 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,3 +33,4 @@ pulldown-cmark = "0.13" ammonia = "4" regex = "1" dotenvy = "0.15" +flate2 = "1" diff --git a/crates/api/Cargo.toml b/crates/api/Cargo.toml index bf8ea506..8820cc8e 100644 --- a/crates/api/Cargo.toml +++ b/crates/api/Cargo.toml @@ -35,3 +35,4 @@ pulldown-cmark.workspace = true ammonia.workspace = true regex.workspace = true dotenvy.workspace = true +flate2.workspace = true diff --git a/crates/api/src/domain/milestones.rs b/crates/api/src/domain/milestones.rs index b02d4373..b1f9bd98 100644 --- a/crates/api/src/domain/milestones.rs +++ b/crates/api/src/domain/milestones.rs @@ -255,11 +255,11 @@ pub async fn repository_milestone_detail_for_actor_by_owner_name( let row = sqlx::query(&milestone_select_sql( "milestones.repository_id = $1 AND milestones.id = $2", )) - .bind(repository.id) - .bind(milestone_id) - .fetch_optional(pool) - .await? - .ok_or(MilestonesError::MilestoneNotFound)?; + .bind(repository.id) + .bind(milestone_id) + .fetch_optional(pool) + .await? + .ok_or(MilestonesError::MilestoneNotFound)?; let summary = summary_from_row(row, &repository)?; let description_html = render_markdown( Some(pool), diff --git a/crates/api/src/domain/organizations.rs b/crates/api/src/domain/organizations.rs index 3c67d5ae..641d320d 100644 --- a/crates/api/src/domain/organizations.rs +++ b/crates/api/src/domain/organizations.rs @@ -1524,9 +1524,7 @@ fn patch_text_required( Ok(normalized) } -fn deserialize_patch_value<'de, D>( - deserializer: D, -) -> Result, D::Error> +fn deserialize_patch_value<'de, D>(deserializer: D) -> Result, D::Error> where D: serde::Deserializer<'de>, { diff --git a/crates/api/src/domain/packages.rs b/crates/api/src/domain/packages.rs index 1fe957e0..1304a0ca 100644 --- a/crates/api/src/domain/packages.rs +++ b/crates/api/src/domain/packages.rs @@ -255,13 +255,21 @@ pub struct PackageCapabilitySummary { #[derive(Debug, Clone, Deserialize, PartialEq, Eq)] #[serde(rename_all = "camelCase", tag = "action")] pub enum PackageSettingsMutation { - UpdateVisibility { visibility: String }, - GrantAccess { username: String, role: String }, + UpdateVisibility { + visibility: String, + }, + GrantAccess { + username: String, + role: String, + }, RevokeAccess { #[serde(rename = "userId")] user_id: Uuid, }, - LinkRepository { owner: String, repo: String }, + LinkRepository { + owner: String, + repo: String, + }, UnlinkRepository { #[serde(rename = "repositoryId")] repository_id: Uuid, diff --git a/crates/api/src/routes/actions.rs b/crates/api/src/routes/actions.rs index ecfe45ba..8a27488f 100644 --- a/crates/api/src/routes/actions.rs +++ b/crates/api/src/routes/actions.rs @@ -6,10 +6,12 @@ use axum::{ routing::{delete, get, patch, post}, Json, Router, }; +use flate2::{write::GzEncoder, Compression}; use serde::Deserialize; use serde_json::json; use sqlx::Row; use std::collections::HashMap; +use std::io::Write; use uuid::Uuid; use crate::{ @@ -1091,15 +1093,23 @@ async fn download_workflow_job_logs_route( Response::builder() .status(StatusCode::OK) - .header(header::CONTENT_TYPE, "text/plain; charset=utf-8") + .header(header::CONTENT_TYPE, "application/gzip") .header( header::CONTENT_DISPOSITION, - format!("attachment; filename=\"{filename}\""), + format!("attachment; filename=\"{filename}.gz\""), ) - .body(Body::from(body)) + .body(Body::from(gzip_bytes(body)?)) .map_err(|_| database_unavailable()) } +fn gzip_bytes(body: String) -> Result, (StatusCode, Json)> { + let mut encoder = GzEncoder::new(Vec::new(), Compression::default()); + encoder + .write_all(body.as_bytes()) + .map_err(|_| database_unavailable())?; + encoder.finish().map_err(|_| database_unavailable()) +} + async fn download_workflow_run_log_archive_route( State(state): State, headers: HeaderMap, diff --git a/crates/api/src/routes/repositories.rs b/crates/api/src/routes/repositories.rs index 783c0186..3123db99 100644 --- a/crates/api/src/routes/repositories.rs +++ b/crates/api/src/routes/repositories.rs @@ -4023,15 +4023,10 @@ async fn create_release( ) -> Result<(StatusCode, Json), (StatusCode, Json)> { let pool = state.db.as_ref().ok_or_else(database_unavailable)?; let actor = AuthenticatedUser::from_headers(&state, &headers).await?.0; - let release = create_repository_release_by_owner_name( - pool, - &owner, - &repo, - Some(actor.id), - request, - ) - .await - .map_err(map_releases_error)?; + let release = + create_repository_release_by_owner_name(pool, &owner, &repo, Some(actor.id), request) + .await + .map_err(map_releases_error)?; Ok((StatusCode::CREATED, Json(json!(release)))) } @@ -4210,15 +4205,10 @@ async fn publish_release( ) -> Result, (StatusCode, Json)> { let pool = state.db.as_ref().ok_or_else(database_unavailable)?; let actor = AuthenticatedUser::from_headers(&state, &headers).await?.0; - let release = publish_repository_release_by_owner_name( - pool, - &owner, - &repo, - release_id, - Some(actor.id), - ) - .await - .map_err(map_releases_error)?; + let release = + publish_repository_release_by_owner_name(pool, &owner, &repo, release_id, Some(actor.id)) + .await + .map_err(map_releases_error)?; Ok(Json(json!(release))) } diff --git a/crates/api/tests/api_repository_actions_job_log_contract.rs b/crates/api/tests/api_repository_actions_job_log_contract.rs index 541ba26a..65ecc4ae 100644 --- a/crates/api/tests/api_repository_actions_job_log_contract.rs +++ b/crates/api/tests/api_repository_actions_job_log_contract.rs @@ -1,8 +1,9 @@ use axum::{ body::{to_bytes, Body}, - http::{header, Method, Request, StatusCode}, + http::{header, HeaderMap, Method, Request, StatusCode}, }; use chrono::{Duration, Utc}; +use flate2::read::GzDecoder; use opengithub_api::{ auth::session, config::{AppConfig, AuthConfig}, @@ -21,6 +22,7 @@ use opengithub_api::{ }; use serde_json::{json, Value}; use sqlx::{PgPool, Row}; +use std::io::Read; use tower::ServiceExt; use url::Url; use uuid::Uuid; @@ -206,6 +208,34 @@ async fn get_text_with_content_type( ) } +async fn get_bytes_with_headers( + app: axum::Router, + uri: &str, + cookie: Option<&str>, +) -> (StatusCode, HeaderMap, Vec) { + let mut builder = Request::builder().method(Method::GET).uri(uri); + if let Some(cookie) = cookie { + builder = builder.header(header::COOKIE, cookie); + } + let request = builder.body(Body::empty()).expect("request should build"); + let response = app.oneshot(request).await.expect("request should run"); + let status = response.status(); + let headers = response.headers().clone(); + let bytes = to_bytes(response.into_body(), usize::MAX) + .await + .expect("body should read"); + (status, headers, bytes.to_vec()) +} + +fn gunzip_utf8(bytes: &[u8]) -> String { + let mut decoder = GzDecoder::new(bytes); + let mut output = String::new(); + decoder + .read_to_string(&mut output) + .expect("gzip body should decompress"); + output +} + async fn seed_run_with_job_logs( pool: &PgPool, owner: &User, @@ -781,6 +811,28 @@ async fn assigned_runner_appends_finalizes_and_streams_job_logs() { assert!(stream_body.contains("run cargo test")); assert!(stream_body.contains("event: cursor")); + let download_uri = format!( + "/api/repos/{}/{}/actions/jobs/{}/logs/download", + owner.email, repo_name, job_id + ); + let (download_status, download_headers, download_body) = + get_bytes_with_headers(app.clone(), &download_uri, Some(&owner_cookie)).await; + assert_eq!(download_status, StatusCode::OK); + assert_eq!( + download_headers + .get(header::CONTENT_TYPE) + .and_then(|value| value.to_str().ok()), + Some("application/gzip") + ); + assert!(download_headers + .get(header::CONTENT_DISPOSITION) + .and_then(|value| value.to_str().ok()) + .unwrap_or_default() + .contains(".log.gz")); + let decompressed_download = gunzip_utf8(&download_body); + assert!(decompressed_download.contains("boot runner")); + assert!(decompressed_download.contains("run cargo test")); + let row = sqlx::query( r#" SELECT job_logs.bytes_written, diff --git a/crates/api/tests/owner_packages_contract.rs b/crates/api/tests/owner_packages_contract.rs index e8fd603d..1a1786c0 100644 --- a/crates/api/tests/owner_packages_contract.rs +++ b/crates/api/tests/owner_packages_contract.rs @@ -386,8 +386,13 @@ async fn organization_packages_show_internal_to_members_only() { ) .await; - let (status, _, public_body) = - get_json(app.clone(), &format!("/api/orgs/{marker}/packages"), None, &client_ip).await; + let (status, _, public_body) = get_json( + app.clone(), + &format!("/api/orgs/{marker}/packages"), + None, + &client_ip, + ) + .await; assert_eq!(status, StatusCode::OK, "{public_body}"); assert_eq!(public_body["total"], 0); diff --git a/crates/api/tests/projects_workspace_contract.rs b/crates/api/tests/projects_workspace_contract.rs index 10c1e168..85b5952a 100644 --- a/crates/api/tests/projects_workspace_contract.rs +++ b/crates/api/tests/projects_workspace_contract.rs @@ -2538,7 +2538,10 @@ async fn project_settings_mutations_enforce_permissions_policy_and_audit_events( ) .await; assert_eq!(status, StatusCode::OK, "{body}"); - assert!(body["teamGrants"].as_array().expect("team grants").is_empty()); + assert!(body["teamGrants"] + .as_array() + .expect("team grants") + .is_empty()); let (status, _, body) = delete_json_body( app, diff --git a/crates/api/tests/repository_network_contract.rs b/crates/api/tests/repository_network_contract.rs index 510d9706..2043f6cc 100644 --- a/crates/api/tests/repository_network_contract.rs +++ b/crates/api/tests/repository_network_contract.rs @@ -116,9 +116,10 @@ async fn cookie_header(pool: &PgPool, config: &AppConfig, user: &User) -> String } async fn get_json(app: axum::Router, uri: &str, cookie: Option<&str>) -> (StatusCode, Value) { - let mut builder = Request::builder() - .uri(uri) - .header("x-forwarded-for", format!("198.51.100.{}", 10 + uri.len() % 100)); + let mut builder = Request::builder().uri(uri).header( + "x-forwarded-for", + format!("198.51.100.{}", 10 + uri.len() % 100), + ); if let Some(cookie) = cookie { builder = builder.header(header::COOKIE, cookie); } diff --git a/web/src/app/[owner]/[repo]/actions/jobs/[jobId]/logs/download/route.ts b/web/src/app/[owner]/[repo]/actions/jobs/[jobId]/logs/download/route.ts index de774ee4..5c34fdb7 100644 --- a/web/src/app/[owner]/[repo]/actions/jobs/[jobId]/logs/download/route.ts +++ b/web/src/app/[owner]/[repo]/actions/jobs/[jobId]/logs/download/route.ts @@ -25,14 +25,18 @@ export async function GET(request: Request, { params }: RouteContext) { const headers = new Headers(); headers.set( "content-type", - response.headers.get("content-type") ?? "text/plain; charset=utf-8", + response.headers.get("content-type") ?? "application/gzip", ); const disposition = response.headers.get("content-disposition"); if (disposition) { headers.set("content-disposition", disposition); } + const encoding = response.headers.get("content-encoding"); + if (encoding) { + headers.set("content-encoding", encoding); + } - return new Response(await response.text(), { + return new Response(await response.arrayBuffer(), { status: response.status, headers, }); diff --git a/web/src/components/RepositoryActionsJobLogPage.tsx b/web/src/components/RepositoryActionsJobLogPage.tsx index 583da554..c00f4a69 100644 --- a/web/src/components/RepositoryActionsJobLogPage.tsx +++ b/web/src/components/RepositoryActionsJobLogPage.tsx @@ -248,11 +248,9 @@ export function RepositoryActionsJobLogPage({ return queryString ? `${jobPath}?${queryString}` : jobPath; } - function goToMatch(nextMatch: number) { - if (!detail.search.query || detail.search.totalMatches < 1) { - return; - } - router.push(searchHref(detail.search.query, nextMatch)); + function navigateTo(href: string) { + window.history.pushState(null, "", href); + router.push(href); } async function copyPermalink(anchor?: string) { @@ -300,8 +298,7 @@ export function RepositoryActionsJobLogPage({ raw: merged.rawLogs, }); setOptionMessage("Saved log options"); - setOptionsOpen(false); - router.push(href); + navigateTo(href); router.refresh(); } catch { setOptionMessage("Could not save log options"); @@ -477,7 +474,7 @@ export function RepositoryActionsJobLogPage({ className="btn primary" href={jobDownloadHref(basePath, detail.job.id)} > - Download log + Download gzip ) : ( @@ -486,13 +483,13 @@ export function RepositoryActionsJobLogPage({ Download run archive )} - {optionMessage ? ( + {optionMessage && !optionsOpen ? (

{optionMessage}

@@ -574,29 +571,48 @@ export function RepositoryActionsJobLogPage({

Steps and output

{ - event.preventDefault(); - router.push(searchHref(searchText)); - }} + method="get" >
setSearchText(event.target.value)} placeholder="Search log" value={searchText} />
- @@ -606,8 +622,9 @@ export function RepositoryActionsJobLogPage({ selectedMatch < 1 || selectedMatch >= detail.search.totalMatches } - onClick={() => goToMatch(selectedMatch + 1)} - type="button" + name="match" + type="submit" + value={String(selectedMatch + 1)} > Next result diff --git a/web/tests/e2e/repository-actions-job-log.spec.ts b/web/tests/e2e/repository-actions-job-log.spec.ts index 87924c63..8153478a 100644 --- a/web/tests/e2e/repository-actions-job-log.spec.ts +++ b/web/tests/e2e/repository-actions-job-log.spec.ts @@ -129,7 +129,7 @@ test("signed-in job log viewer renders job sidebar, steps, and annotations", asy "opengithub workflow log archive", ); await expect( - page.getByRole("link", { name: "Download log" }), + page.getByRole("link", { name: "Download gzip" }), ).toHaveAttribute("href", /\/actions\/jobs\/.*\/logs\/download/); await expectNoDeadControls(page); diff --git a/web/tests/repository-actions-job-log.test.tsx b/web/tests/repository-actions-job-log.test.tsx index b5882827..b4c5ce4f 100644 --- a/web/tests/repository-actions-job-log.test.tsx +++ b/web/tests/repository-actions-job-log.test.tsx @@ -333,7 +333,7 @@ describe("RepositoryActionsJobLogPage", () => { ).length, ).toBeGreaterThan(0); expect(screen.getByText("Type error")).toBeVisible(); - expect(screen.getByRole("link", { name: "Download log" })).toHaveAttribute( + expect(screen.getByRole("link", { name: "Download gzip" })).toHaveAttribute( "href", "/mona/octo-app/actions/jobs/job-1/logs/download", ); @@ -415,13 +415,11 @@ describe("RepositoryActionsJobLogPage", () => { }), }), ); - expect(await screen.findByText("Saved log options")).toBeVisible(); + expect((await screen.findAllByText("Saved log options"))[0]).toBeVisible(); expect(pushMock).toHaveBeenCalledWith( "/mona/octo-app/actions/runs/run-1/jobs/job-1?q=error&match=1×tamps=false&raw=true", ); - expect(screen.queryByRole("menu")).not.toBeInTheDocument(); - - fireEvent.click(screen.getByRole("button", { name: "Log options" })); + expect(screen.getByRole("menu")).toBeVisible(); fireEvent.click( screen.getByRole("menuitem", { name: "Copy job permalink" }), ); @@ -442,18 +440,28 @@ describe("RepositoryActionsJobLogPage", () => { fireEvent.change(screen.getByRole("textbox", { name: "Search log" }), { target: { value: "cache miss" }, }); - fireEvent.click(screen.getByRole("button", { name: "Search" })); - expect(pushMock).toHaveBeenCalledWith( - "/mona/octo-app/actions/runs/run-1/jobs/job-1?q=cache+miss&match=1×tamps=false", + expect(screen.getByRole("textbox", { name: "Search log" })).toHaveValue( + "cache miss", + ); + expect(screen.getByRole("button", { name: "Search" })).toHaveAttribute( + "value", + "1", ); - fireEvent.click(screen.getByRole("button", { name: "Next result" })); - expect(pushMock).toHaveBeenCalledWith( - "/mona/octo-app/actions/runs/run-1/jobs/job-1?q=error&match=2×tamps=false", + expect(screen.getByRole("button", { name: "Next result" })).toHaveAttribute( + "value", + "2", ); expect( screen.getByRole("button", { name: "Previous result" }), ).toBeDisabled(); + expect(document.querySelector('input[name="timestamps"]')).toHaveAttribute( + "value", + "false", + ); + expect( + screen.getByRole("button", { name: "Search" }).closest("form"), + ).toHaveAttribute("action", "/mona/octo-app/actions/runs/run-1/jobs/job-1"); }); it("renders highlighted matches and line permalink copy actions", async () => { @@ -504,7 +512,9 @@ describe("RepositoryActionsJobLogPage", () => { expect( within(status).getByText("workflow logs are unavailable"), ).toBeVisible(); - expect(screen.getByRole("button", { name: "Download log" })).toBeDisabled(); + expect( + screen.getByRole("button", { name: "Download gzip" }), + ).toBeDisabled(); }); it("shows backend validation errors while preserving the job shell", () => {