Skip to content

Remove duplicate Skill Value sample column - #1148

Open
AbhitejJohn wants to merge 2 commits into
mainfrom
abhitejjohn-surface-skill-value
Open

Remove duplicate Skill Value sample column#1148
AbhitejJohn wants to merge 2 commits into
mainfrom
abhitejjohn-surface-skill-value

Conversation

@AbhitejJohn

@AbhitejJohn AbhitejJohn commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • show one shared sample-count column for token and time deltas
  • keep group rows and drill-down spans aligned with the seven-column table
  • add a regression test for the Skill Value table structure

Before

Two identical N columns appear after the token and time deltas.

Skill Value dashboard before

After

One shared N column reports the paired observation count used by both deltas.

Skill Value dashboard after

Validation

  • node --test eng\dashboard\*.test.js
  • node --check eng\dashboard\skill-value.js
  • node --check eng\dashboard\skill-value.test.js
  • git diff --check

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3f54e78b-35cc-4a23-923e-fae37fb232d1
Copilot AI lite review requested due to automatic review settings September 9, 2026 17:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new test mutates global state without robust cleanup and should be adjusted to avoid leaking globals and to fail with clearer assertions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 Medium severity · 1 Low severity

New issues introduced by this change (2)
Severity Finding
Medium severity eng/​dashboard/​skill-value.test.js — This test overwrites globalThis.document/window/fetch but never restores the previous values, which…
Low severity eng/​dashboard/​skill-value.test.jswrap.innerHTML.match(...)[1] will throw a TypeError (instead of a clear assertion failure) if the…
What changed in this PR

This PR updates the Skill Value dashboard table to remove a duplicated sample-count (“n”) column by using a single shared paired-observation count for both token and time deltas, and adds a regression test to lock in the new 7-column structure.

Changes:

  • Remove the duplicate “n” column so token/time deltas share one paired-sample count.
  • Update group/detail row column counts (<td> blanks, colspan) to align with the new 7-column table.
  • Add a Node-based regression test validating the table header structure and colspan.
File Description
eng/​dashboard/​skill-value.js Removes duplicate sample-count column and realigns group/detail rows to 7 columns.
eng/​dashboard/​skill-value.test.js Adds regression coverage for the Skill Value table header/colspan structure.
Suppressed comments (1)

eng/dashboard/skill-value.test.js:63

  • After restoring globals at setup time, also restore them at the end of the test so subsequent tests start from a clean environment (ideally this would be in a finally block).
});

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread eng/dashboard/skill-value.test.js
Comment thread eng/dashboard/skill-value.test.js Outdated
@github-actions github-actions Bot added the waiting-on-author PR state label label Sep 9, 2026
@AbhitejJohn
AbhitejJohn requested a review from y87feng September 9, 2026 17:36
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

👋 @AbhitejJohn — this PR has 2 unresolved review thread(s). When you're ready, please address the feedback and push an update; the triage bot will pick up the next state automatically. (Add the no-stale label to silence further pings.)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3f54e78b-35cc-4a23-923e-fae37fb232d1
Copilot AI review requested due to automatic review settings September 9, 2026 20:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The column removal is consistently applied across header/body/group/drill-down rows and is backed by a focused regression test that asserts the intended 7-column structure.

Review tier: Lite
Findings: None

Issues resolved since last review (2)
Severity Finding
Low severity eng/​dashboard/​skill-value.test.jswrap.innerHTML.match(...)[1] will throw a TypeError (instead of a clear assertion failure) if the… View resolved comment
Medium severity eng/​dashboard/​skill-value.test.js — This test overwrites globalThis.document/window/fetch but never restores the previous values, which… View resolved comment

@github-actions github-actions Bot added waiting-on-review PR state label and removed waiting-on-author PR state label labels Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

✅ Evaluation passed for 92b7b4f. cc @AbhitejJohn @JanKrivanek — please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-review PR state label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants