Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/src/bindings/ProjectPlan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,4 @@ agent: AgentRun | null,
* file is read back and parsed into [`Self::doc`]. `None` for plans that
* were loaded directly from a document rather than produced by an agent.
*/
plan_path?: string, };
plan_path: string | null, };
6 changes: 3 additions & 3 deletions app/src/bindings/Review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,12 @@ project: ProjectId | null,
* Overwritten on each dispatch (see [`DispatchSnapshot`]). `None` before
* the first dispatch and for legacy data.
*/
dispatch_snapshot?: DispatchSnapshot,
dispatch_snapshot: DispatchSnapshot | null,
/**
* Rolled-up CI status for this review's PR, when fetched. `None` until a
* CI check read populates it, and for legacy data that predates the field.
*/
ci_summary?: CiSummary,
ci_summary: CiSummary | null,
/**
* Advisory findings from the read-only pre-pass reviewer, if it has run.
*
Expand All @@ -135,4 +135,4 @@ conversation: Array<ConversationItem>,
*
* `None` before the first pre-pass and for legacy data.
*/
last_reviewed_sha?: string, };
last_reviewed_sha: string | null, };
2 changes: 1 addition & 1 deletion app/src/components/ReviewCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ function RiskChips({ review }: { readonly review: Review }) {

return (
<>
{ci !== undefined && ci.total > 0 && (
{ci !== null && ci.total > 0 && (
<RiskChip
tone={ciSummaryTone(ci)}
title={`CI: ${String(ci.passed)} passed, ${String(ci.failed)} failed, ${String(ci.pending)} pending`}
Expand Down
33 changes: 33 additions & 0 deletions app/src/lib/attention.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,36 @@ describe("isFastLane", () => {
expect(isFastLane(root)).toBe(true);
});
});

describe("null ci_summary (wire shape regression)", () => {
// Serde serializes Option::None as `null` with the key present. The board
// crashed white when attentionRank dereferenced a null summary (the old
// guard only handled `undefined`). These lock the null path.
it("ranks a review whose ci_summary is null without throwing", () => {
const review = makeReview({ ci_summary: null });
expect(() => attentionRank(review)).not.toThrow();
// Null CI must rank identically to "no checks loaded" — no adjustment.
const withCi = makeReview({
id: "rev-ci",
ci_summary: { passed: 1, failed: 0, pending: 0, total: 1 },
});
expect(attentionRank(review)).toBeGreaterThan(attentionRank(withCi));
});

it("excludes a null-CI review from the fast lane (absent CI never qualifies)", () => {
const review = makeReview({ ci_summary: null, parents: [] });
expect(isFastLane(review)).toBe(false);
});

it("sorts a mixed board of null and populated summaries without throwing", () => {
const reviews = [
makeReview({ id: "a", ci_summary: null }),
makeReview({
id: "b",
ci_summary: { passed: 2, failed: 1, pending: 0, total: 3 },
}),
makeReview({ id: "c", ci_summary: null, stale: true }),
];
expect(() => sortByAttention(reviews)).not.toThrow();
});
});
4 changes: 3 additions & 1 deletion app/src/lib/attention.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ const SENSITIVE = 0.15; // auth/migrations/config/etc. warrant scrutiny — sink

/** The rolled-up CI state for a review, or `"none"` when no checks are loaded. */
function reviewCiState(review: Review): CiState {
// Serde serializes `Option::None` as `null` (the key is always present on
// the wire), so the empty state is `null` — never `undefined`.
const ci = review.ci_summary;
return ci === undefined ? "none" : ciState(ci);
return ci === null ? "none" : ciState(ci);
}

/** Within-bucket adjustment from CI status. */
Expand Down
6 changes: 6 additions & 0 deletions app/src/test/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ export function makeReview(overrides: Partial<Review> = {}): Review {
project: null,
review_findings: [],
conversation: [],
// Serde emits `Option::None` as `null` with the key present — fixtures
// must mirror the wire shape, not omit the fields (omission hid a
// null-deref crash the type system couldn't see).
dispatch_snapshot: null,
ci_summary: null,
last_reviewed_sha: null,
};
return { ...base, ...overrides };
}
Expand Down
4 changes: 0 additions & 4 deletions crates/cockpit-core/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,6 @@ pub struct ProjectPlan {
/// file is read back and parsed into [`Self::doc`]. `None` for plans that
/// were loaded directly from a document rather than produced by an agent.
#[serde(default)]
#[ts(optional)]
pub plan_path: Option<PathBuf>,
}

Expand Down Expand Up @@ -524,12 +523,10 @@ pub struct Review {
/// Overwritten on each dispatch (see [`DispatchSnapshot`]). `None` before
/// the first dispatch and for legacy data.
#[serde(default)]
#[ts(optional)]
pub dispatch_snapshot: Option<DispatchSnapshot>,
/// Rolled-up CI status for this review's PR, when fetched. `None` until a
/// CI check read populates it, and for legacy data that predates the field.
#[serde(default)]
#[ts(optional)]
pub ci_summary: Option<CiSummary>,
/// Advisory findings from the read-only pre-pass reviewer, if it has run.
///
Expand All @@ -545,7 +542,6 @@ pub struct Review {
///
/// `None` before the first pre-pass and for legacy data.
#[serde(default)]
#[ts(optional)]
pub last_reviewed_sha: Option<String>,
}

Expand Down
Loading