fix: improve responsive behavior of AssetMapper and EntryMapper table…#1118
Conversation
…s to ensure pagination and footer remain visible on zoom
🔒 Security Scan Results
⏱️ SLA Breach Summary
🔴 Critical Severity - SLA Breached Issues (with fixes)Showing 1 issue(s) that have exceeded the 15-day SLA threshold:
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
❌ BUILD FAILED - Security checks failed Please review and fix the security vulnerabilities before merging. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
✅ BUILD PASSED - All security checks passed |
🔒 Security Scan Results
⏱️ SLA Breach Summary
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
✅ BUILD PASSED - All security checks passed |
umesh-more-cstk
left a comment
umesh-more-cstk
left a comment
There was a problem hiding this comment.
Review — CMG-1044 responsive AssetMapper/EntryMapper tables
Solid, well-documented fix for a genuinely awkward layout problem — the extensive comments explaining the feedback-loop avoidance and the calc(100vh - 246px) reserve are appreciated and made this reviewable. The bounded-flex + measured-tableHeight approach is sound, and observing the fixed-size outer box (not .Table/.Table__body) correctly avoids the runaway loop. Only one mapper mounts at a time (ternary in entryAssetMapper.tsx), so the global document.querySelector reads don't cross-match between the two.
Verdict: Comment — no blockers. Two things I'd like addressed before merge (both inline): extract the duplicated measurement effect into a shared hook, and confirm the entry-table column-width change doesn't trade the vertical overflow for a horizontal one at the same 110%+ zoom this targets.
Please address before merge
- Duplicated measurement effect (
entryMapper.tsx↔assetMapper.tsx) — inline. equalWidthColumnsflip + fixed column widths — verify no horizontal overflow — inline.
Non-blocking nits
box?.clientHeight ?? …doesn't catch0;avail > 80leaves a stale height at extreme zoom (inline).- Magic numbers
246/520/64/65/56are duplicated across both files and the SCSS. The initial state usesinnerHeight - 520but the box fallback usesinnerHeight - 246— worth a shared named constant, and the two seeds reconciled. - The
window.addEventListener('resize', measure)is largely redundant with theResizeObserveron a100vh-based box (both fire on zoom). Harmless, just noise. index.scss: the removed.Tablerule leaves a double blank line, and the file has no trailing newline (lint should catch both).
…redTableHeight for responsive table height
🔒 Security Scan Results
⏱️ SLA Breach Summary
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
✅ BUILD PASSED - All security checks passed |
umesh-more-cstk
left a comment
umesh-more-cstk
left a comment
There was a problem hiding this comment.
Review: fix: improve responsive behavior of AssetMapper and EntryMapper tables (CMG-1044)
Verdict: COMMENT (non-gating). This is a well-engineered layout fix. I traced the full flex chain at the head commit and it holds together: .entry-asset-mapper (bounded to calc(100vh - 246px), overflow:hidden) → .step-container (flex:1) → .table-container (flex:1 1 auto, align-items/content:stretch) → .content-types-fields-wrapper (flex col) → .entry-table-wrapper (flex:1 1 auto) → .entry-mapper-table → .Table + .mapper-footer. The measured-height approach (feed react-window's tableHeight off the bounded outer box, never off .Table/.Table__body) is the right way to avoid the feedback loop, and the effect cleanup (ResizeObserver disconnect + resize listener removal) is correct.
I could not find a blocking issue. One thing I checked and disproved: I suspected the reserve in useMeasuredTableHeight omits the .locale-select-inline row and would clip the footer on multi-locale projects — but that row is position:absolute (overlaid on .TablePanel, which reserves space via min-height:64px; padding-right:272px), so it does not consume flex height. No bug there.
Non-blocking (inline):
- nit —
useMeasuredTableHeight.ts:44: initialtableHeightguess is unclamped and inconsistent with the hook's own box model. - question —
entryMapper.tsx:656: entry columns switched from equal-width to fixed non-shrinking widths (a horizontal-layout change bundled with the vertical fix) — please confirm no horizontal overflow at the min supported width.
Out-of-diff heads-ups (not blocking):
- Tests / ResizeObserver:
new ResizeObserver(...)is called unconditionally, butui/tests/setup.tsdoes not polyfill it. The only ContentMapper tests today are*.utils.test.ts(pure functions) — no test mountsEntryMapper/AssetMapper— so CI is unaffected now. If a component render test is ever added it will throwReferenceError: ResizeObserver is not definedunder jsdom; worth a stub intests/setup.tspreemptively. - Description vs diff: the PR text says it "Added the
entry-asset-mapperclass to the Entry/Asset toggle wrapper (it had none)". In the code,.entry-asset-mapperis already on the outer wrapper inentryAssetMapper.tsx(.mapper-view-toggleis the toggle), and that file is not part of this diff — the description likely reflects an earlier commit on the branch. No code impact.
Verified good: dev-only console.warn on missing selectors instead of silently falling back to magic constants (directly addresses this repo's "DOM reads by hard-coded class names that silently fall back to magic numbers" concern); DOM reads scoped to wrapperRef rather than global document; the empty-state height rule (.Table:has(.EmptyState), 4-class specificity) still wins over the new .entry-mapper-table .Table { height:auto }, so the empty state is preserved; and the entry/asset .Table__body rules don't cross-contaminate. Scope is clean — UI-only, matches the stated Affected Area, no lockfile/dependency churn.
…ss and update initial table height calculation
🔒 Security Scan Results
⏱️ SLA Breach Summary
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
✅ BUILD PASSED - All security checks passed |
🔗 Jira Ticket
CMG-1044
📋 PR Type
📝 Description
What changed?
.entry-asset-mapperis now pinned to the real step-content area (calc(100vh - 246px), derived from the stepper's own.stepContentheight minus its fixed header padding), so the subtree is self-bounding instead of growing to content height.InfiniteScrollTable'stableHeightprop via aResizeObserver, so react-window's virtual viewport matches the visible area and its native scroll works. Anchored on the outer box (never on.Table/.Table__body) to avoid a measurement feedback loop.calc(100vh - Npx)/ hardcodedtableHeight={400}sizing with flex + measured height; the table body scrolls internally while pagination + Save stay pinned below.entry-asset-mapperclass to the Entry/Asset toggle wrapper (it had none, so its flex layout rules never applied) and removed dead.asset-mapper-container .Tablefixed-height rules.Why?
On zoom (110%+), the fixed-pixel/
100vh-based table height didn't shrink with the effective viewport, so the pagination bar slid below the Save footer and the Save button became unreachable — the UI locked with no way to scroll to it. This makes the layout distribute height correctly and keep the controls in view at 100%, 110% and beyond.🧩 Affected Areas
api— Node.js backendui— React frontendupload-api— Upload API serverdocker/docker-compose🧪 How to Test
Expected result: At every zoom level the table body scrolls internally, and the pagination bar and Save button stay visible and reachable at the bottom (no overflow, no locked UI). Repeat on the Assets tab.
📸 Screenshots / Recordings
🔗 Related PRs / Dependencies
✅ Author Checklist
feature/,bugfix/, orhotfix/+ 5–30 lowercase chars.env/example.envupdated if new environment variables were added — N/A (no env changes)npm test)README.md/ docs updated if behaviour changed — N/A👀 Reviewer Notes
The table body height is driven by a measured JS value (
tableHeightprop), not CSS, becauseInfiniteScrollTable/react-window sizes its virtual scroll viewport from that number. The measurement deliberately anchors on the bounded outer box (.entry-asset-mapper) and never reads.Table/.Table__body— reading those creates a runaway feedback loop (we sawtableHeightblow up to ~9966px during development). Please sanity-check thecalc(100vh - 246px)reserve if the surrounding stepper chrome changes.