fix: default heatmap reference to first after label#12
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34ea82a17e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| export function defaultHeatmapReferenceLabel(frames: WebUploadFramePlan[]) { | ||
| return frames[0]?.after.label ?? "After"; |
There was a problem hiding this comment.
When a scan mixes primary output variants across frames, e.g. frame 1 has *_output.png while frame 2 only has *_rip.png, the new variant-specific after.label values become After and Rip; this default still picks only the first frame's label. generateUploadFrames later calls heatmapAfterAsset for every frame and throws when that label is absent, while the UI's heatmap options intentionally only allow labels common to all frames. Please derive this default from the same common-label set, or add an issue/fallback instead of storing a label that some frames cannot use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates the internal-site web uploader’s source scanning logic so heatmapReferenceLabel defaults to the first frame’s actual after.label (instead of always "After"), and ensures variant-derived labels (e.g. "Rip") are preserved for primary after assets so downstream heatmap selection can find a real comparison column.
Changes:
- Add
defaultHeatmapReferenceLabel(frames)and use it in both flat and nested upload plan builders. - Update
assetPlan(..., kind: "after")to usevariantLabel(candidate.variant)so primary after labels can be"Rip"/etc when appropriate. - Refine nested after-directory variant override handling and add a unit test for
src/rip/fltnested sets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/internal-site/components/web-uploader/source-scanner.ts | Computes default heatmap reference from the first frame’s after label; preserves variant-derived labels for primary after assets; adjusts nested after-dir variant overrides. |
| apps/internal-site/components/web-uploader/source-scanner.test.ts | Adds coverage asserting src/rip/flt yields after.label === "Rip" and heatmapReferenceLabel === "Rip". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const afterParsedResults = layout.afterDirs.map((directory) => { | ||
| const directoryVariant = basename(directory).toLowerCase(); | ||
| return parseEntries( | ||
| scopedEntries(directory), | ||
| PRIMARY_AFTER_VARIANTS.has(directoryVariant) ? "out" : directoryVariant, | ||
| ); | ||
| }); |
| it("uses the first after label as the default heatmap reference for src/rip/flt sets", () => { | ||
| const plan = scanBrowserUploadFiles( | ||
| [ | ||
| image("case/src/001.png"), | ||
| image("case/rip/001.png"), |
### Motivation - Ensure the plan's `heatmapReferenceLabel` points at an actual comparison column instead of the hardcoded string `"After"` so downstream heatmap generation does not fail. - Preserve variant-derived labels (e.g. `Rip`) for primary after assets so nested imports like `src`/`rip` become valid global references. ### Description - Added exported helper `defaultHeatmapReferenceLabel(frames: WebUploadFramePlan[])` that returns the first frame's `after.label` and falls back to `"After"` when there are no frames. - Use `defaultHeatmapReferenceLabel(frames)` instead of the hardcoded `heatmapReferenceLabel: "After"` in both `buildFlatPlan` and `buildNestedPlan`. - Preserve variant-derived labels for primary after assets by making `assetPlan(..., kind: 'after')` use `variantLabel(candidate.variant) || "After"` instead of always `"After"`. - Improve nested after-directory parsing so non-primary directory names (e.g. `rip`) are passed as the variant override while mapping primary after/output dirs to the standard after variant. - Added/updated test in `source-scanner.test.ts` to assert that a `src`/`rip`/`flt` set produces `frames[0].after.label === "Rip"` and `plan.heatmapReferenceLabel === "Rip"`. ### Testing - Ran unit tests for the internal-site package with `pnpm --filter @magic-compare/internal-site test -- source-scanner.test.ts`, and the test run completed successfully (all tests passed). - Ran lint with `pnpm --filter @magic-compare/internal-site lint`, which succeeded with no errors.
|
@copilot review update |
Updated. The review feedback has been addressed in commit |
Motivation
heatmapReferenceLabelpoints at an actual comparison column instead of the hardcoded string"After"so downstream heatmap generation does not fail.Rip) for primary after assets so nested imports likesrc/ripbecome valid global references.Description
defaultHeatmapReferenceLabel(frames: WebUploadFramePlan[])that returns the first frame'safter.labeland falls back to"After"when there are no frames.defaultHeatmapReferenceLabel(frames)instead of the hardcodedheatmapReferenceLabel: "After"in bothbuildFlatPlanandbuildNestedPlan.assetPlan(..., kind: 'after')usevariantLabel(candidate.variant) || "After"instead of always"After".rip) are passed as the variant override while mapping primary after/output dirs to the standard after variant.source-scanner.test.tsto assert that asrc/rip/fltset producesframes[0].after.label === "Rip"andplan.heatmapReferenceLabel === "Rip".Testing
pnpm --filter @magic-compare/internal-site test -- source-scanner.test.ts, and the test run completed successfully (all tests passed).pnpm --filter @magic-compare/internal-site lint, which succeeded with no errors.Codex Task