Skip to content

#666 Redesign The Edit Position Page - #690

Merged
b-at-neu merged 13 commits into
devfrom
666-redesign-the-edit-position-page
Sep 12, 2026
Merged

#666 Redesign The Edit Position Page#690
b-at-neu merged 13 commits into
devfrom
666-redesign-the-edit-position-page

Conversation

@b-at-neu

@b-at-neu b-at-neu commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Closes #666

Summary

  • Redesigns /manage/positions/[id]/edit as one max-w-5xl scrolling page of SectionCards (a merged Details+Availability card, Managers, Questions, Danger zone) — no tabs.
  • Scope change (post-approval): per direct human feedback given after this PR's first review cycle ("Lets cut the applications section entirely"), the Applications summary section was added and then removed within this PR — position-applications-summary.tsx was deleted and never shipped. Redesign The Edit Position Page #666's acceptance criteria and plan have been updated to reflect this; it is intentional, not an oversight.
  • Moves position status out of the form and into PageHeader header actions (a split button + overflow, mirroring /manage/applications/[id]'s status control), so illegal transitions are simply never offered instead of reachable-then-refused.
  • Replaces the whole-form updatePosition submit with four narrow, auto-saving server actions (updatePositionTitle, updatePositionDescription, updatePositionSchedule, updatePositionStatus) — every field saves on blur with inline save/error state, no per-field toast spam.
  • Fixes two existing bugs: managers were editable on an archived position (now gated like every other field, with a new PositionManagersReadonly view), and warning copy referenced field position ("below") rather than the field itself.
  • Scope change (post-approval, human feedback): the draft → open and open → closed header buttons are renamed Open position and Close position (were Publish and Close applicationsclosed → open's Reopen is unchanged); Open position now always renders for a manager too (disabled with a tooltip), matching the pattern already built for Reopen; SectionCard's header padding is tightened site-wide; a full-width live-edit warning bar now shows under the page title for open/closed positions.

Changes

  • app/(main)/(auth)/manage/positions/[id]/edit/page.tsx + loading.tsx — full restructure at the Wide (max-w-5xl) tier; now also renders a live-edit warning bar for open/closed positions.
  • New: lib/use-autosave.ts, components/features/position-details-section.tsx, position-availability-section.tsx, position-status-header-actions.tsx, position-managers-readonly.tsx. (position-applications-summary.tsx was added then deleted — see Scope change above.)
  • prisma/actions/position-actions.tsupdatePosition replaced by the four field actions sharing authorizePositionEdit/revalidatePositionSurfaces helpers; addPositionManager/removePositionManager now also gate on checkPositionEditable.
  • lib/constants.ts — split positionFormShape into reusable per-field schemas; added getPositionTransitionTargets, POSITION_TRANSITION_ACTIONS, POSITION_OPEN_REQUIRES_ADMIN_NOTE (renamed from POSITION_PUBLISH_REQUIRES_ADMIN_NOTE), POSITION_REOPEN_REQUIRES_ADMIN_NOTE, POSITION_CLOSED_DRAFT_BLOCKED_ERROR, POSITION_LIVE_EDIT_WARNING; moved PIPELINE_STATUSES to the shared APPLICATION_PIPELINE_STATUSES; removed closed → draft from POSITION_STATUS_TRANSITIONS; renamed the draft → open / open → closed action labels to Open position / Close position; removed the now-unreachable POSITION_UNPUBLISH_BLOCKED_HINT note branch.
  • components/features/markdown-field.tsxdisabled/footer/onCommit props, minimal { description } context type.
  • components/ui/section-card.tsx — new 'form-field' skeleton row shape; tightened header padding (pt-4 pb-3pt-3 pb-2) site-wide, applying to every SectionCard consumer.
  • Deleted: position-edit-tabs.tsx, position-details-form.tsx, position-details-readonly.tsx.
  • Docs: docs/DESIGN.md (width tier), docs/WORKFLOWS.md (PM-4–PM-7, AD-2), docs/PERMISSIONS.md (manager archived-gate reversal, action table, button-label renames, live-edit warning).
  • Tests: rewrote tests/unit/position-transitions.test.ts around getPositionTransitionTargets; added POSITION_TRANSITION_ACTIONS/schedule-validation cases to tests/unit/constants.test.ts; migrated tests/db/position-transitions.test.ts / position-publish-permission.test.ts to updatePositionStatus; added tests/db/position-field-saves.test.ts; added archived-manager cases to tests/db/position-archive.test.ts.

Testing plan

  • As an admin on a draft: Open position → confirm → toast "Position opened", badge flips to Open, the action bar becomes Close position + caret.
  • As a manager on the same draft (before publishing): Open position is shown disabled, with a tooltip (hover or keyboard focus) reading "Only an admin can open this position."
  • Edit the title, tab away → "Saving…" then "Saved"; reload — it persisted. Clear the title and tab away → "Title is required" inline, nothing saved.
  • Edit the description, toggle Preview, tab away → saved. Paste >10,000 characters → inline error, no save.
  • Set a close date before the open date → the ordering message on both fields, nothing saved. Fix it → saved.
  • Type a partial date and tab away → no "must be today or later" error fires mid-typing. Change one date alone on an already-open position whose opensAt is in the past → only the changed field is checked.
  • On a closed position whose close date has passed: Reopen is shown disabled, with a tooltip (hover or keyboard focus) explaining the close date has passed; extend the close date, wait for the save, reload → Reopen is now enabled.
  • As a manager on a closed position: Reopen is shown disabled with a tooltip explaining only an admin can reopen it; there is no Return to draft option at all on a closed position, for either role.
  • On an open position with applications: Return to draft is absent; Close position confirms with the unresolved count.
  • As a manager on an archived position: no header actions, read-only Details/Availability/Questions, and the managers list has no remove button or search box. As an admin on the same position: everything is editable, including managers.
  • Two browsers, two managers: A edits the title, B edits the description — both persist; neither clobbers the other.
  • Keyboard only: tab through every field, confirm each save status is announced, open the caret menu with Enter, Escape closes it, focus returns to the trigger; the disabled Open position/Reopen button is reachable and announces its tooltip on keyboard focus too.
  • Both themes at 375 / 768 / 1280 px — sections stack, header actions wrap, no horizontal scroll; the live-edit warning bar spans full width and wraps cleanly at 375px.
  • A stale link to a deleted position on any field save → "This position no longer exists."
  • A manager attempting addPositionManager/removePositionManager on an archived position → ARCHIVED_POSITION_EDIT_ERROR; an admin's succeeds.
  • Open a draft position → no live-edit warning bar. Open an open or closed position → the warning bar appears under the page title, reading "Changes made here are immediately visible in the live application."
  • Compare header padding on /dashboard, /applications/[id] and /manage/applications/[id] SectionCards before/after — the tighter padding reads as a deliberate, consistent tightening, not a regression.

Automated checks

  • npm run prettier:check — pass
  • npm run eslint:check — pass
  • npm run tsc:check — pass
  • npm run test:unit — pass (376/376; tests/unit/email-delivery-events.test.ts fails in this sandbox only because DATABASE_URL isn't set locally — confirmed pre-existing on dev, unrelated to this change)
  • npm run build — compiles and type-checks cleanly; the page-data-collection step needs a real Postgres connection (/api/webhooks/resend), unavailable in this sandbox — confirmed the identical failure exists on unmodified dev
  • tests/db/* — not run locally (no Postgres available in this environment); will run in CI

Notes

  • The PositionManagersSection capability reversal (managers no longer editable on an archived position) is a deliberate policy change called out in the plan's risks — docs/PERMISSIONS.md is updated accordingly.
  • Reopening a closed position past its close date is now a visible two-step (extend the date, then Reopen becomes enabled) rather than one combined submit — this is the correct trade per the plan (the server never trusts an unsaved value for closesAtPast).
  • Scope change (human decision after cycle 5 escalation): closed → draft is no longer a legal transition at all (POSITION_STATUS_TRANSITIONS), independent of closesAt or application count — reopening is the only way out of closed. Reopen is now always rendered for a closed position rather than conditionally hidden — it renders disabled with a keyboard-reachable tooltip explanation (admin-only, or past close date) instead of disappearing. docs/PERMISSIONS.md and docs/WORKFLOWS.md are updated accordingly; see the PR discussion for the full decision.
  • Scope change (post-approval human feedback, this revision): after this PR's approval, direct human feedback requested five further changes, applied in this revision: (1) tightened SectionCard's shared header padding, resolving the earlier-deferred R3-L1 finding; (2)/(3) renamed the open/close header buttons to Open position / Close position for a consistent, plain-language pair; (4) extended the always-rendered-but-disabled-with-tooltip pattern (already built for Reopen) to Open position, so a manager sees why they can't open a position instead of just not seeing a button; (5) added a full-width live-edit warning bar for open/closed positions. docs/WORKFLOWS.md and docs/PERMISSIONS.md are updated accordingly.

@b-at-neu b-at-neu added the claude Will be worked on by Claude label Sep 8, 2026
@b-at-neu b-at-neu self-assigned this Sep 8, 2026
@vercel

vercel Bot commented Sep 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
aplio Ready Ready Preview Sep 12, 2026 3:36pm UTC

@b-at-neu b-at-neu added ready for review PR ready for review agent reviewing Review agent working (in-flight) and removed ready for review PR ready for review agent labels Sep 8, 2026

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Code Review — Cycle 1 · needs revision

7 open — 1 🔴 Critical, 1 🟠 Medium, 5 🟡 Low (see inline)

🔴 R1-C1 — this PR is unmergeable against dev right now (mergeable: CONFLICTING, mergeStateStatus: DIRTY, 11 commits behind), and as a direct consequence zero required GitHub Actions checks exist for this commit (a4e6137) — no run-prettier-check/run-linting-check/run-tsc-check/run-commit-message-check/Build/Test/Neon-branch-check have run at all; only the unrelated Vercel checks report. None of the automated checks the PR description claims to pass (prettier:check, eslint:check, tsc:check, test:unit) have actually been verified by CI on this SHA. Rebase onto dev so CI can run and this can be evaluated for merge.

Comment thread prisma/actions/position-actions.ts Outdated
Comment thread lib/use-autosave.ts Outdated
Comment thread components/features/position-status-header-actions.tsx Outdated
Comment thread prisma/actions/position-actions.ts Outdated
Comment thread components/features/position-availability-section.tsx Outdated
Comment thread lib/constants.ts Outdated
@b-at-neu b-at-neu added needs revision Review found issues that need fixing revising Revise agent working (in-flight) and removed reviewing Review agent working (in-flight) needs revision Review found issues that need fixing labels Sep 8, 2026
@b-at-neu
b-at-neu force-pushed the 666-redesign-the-edit-position-page branch from a4e6137 to 5964daf Compare September 8, 2026 02:23
@b-at-neu

b-at-neu commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Revision — Cycle 1

fixed R1-M1, R1-L1, R1-L2, R1-L3, R1-L4, R1-L5 · 5964daf · rebase: lib/constants.ts (dual independent tail additions, union-merged), tests/unit/constants.test.ts (dev's unused-import deletion kept, PR's new type import kept)

@b-at-neu b-at-neu added ready for review PR ready for review agent reviewing Review agent working (in-flight) and removed revising Revise agent working (in-flight) ready for review PR ready for review agent labels Sep 8, 2026

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Code Review — Cycle 2 · approved

0 open — all Cycle 1 findings resolved (R1-C1 rebase/CI, R1-M1 updateMany race, R1-L1..L5 comment length), no regressions found in the delta.

@b-at-neu b-at-neu added approved Review passed, ready to merge needs revision Review found issues that need fixing and removed reviewing Review agent working (in-flight) approved Review passed, ready to merge labels Sep 8, 2026
@b-at-neu

b-at-neu commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Human feedback on the redesigned edit-position page

Five changes, in order of the page layout:

1. Merge Availability into Details. Combine the "Availability" SectionCard into the "Details" SectionCard — one card, not two. Keep PositionDetailsSection and PositionAvailabilitySection as separate components if that's the lowest-risk path (each already has its own autosave), just render both inside a single SectionCard title="Details" in app/(main)/(auth)/manage/positions/[id]/edit/page.tsx. Fold availabilityWarnings in at the top of that merged card, above the fields.

2. More side-by-side, less vertical stacking. Two concrete opportunities:

  • Within the merged Details card: Title (full width, since titles can be long) → Opens At / Closes At side by side (already grid sm:grid-cols-2 in PositionAvailabilitySection — keep that) → Description last, full width.
  • Managers and Questions are both list-shaped sections of similar height — put them side by side in a grid gap-4 md:grid-cols-2 on medium+ screens, stacking on mobile as normal. Danger zone stays its own full-width section at the bottom.

3. Decrease the padding underneath card titles. components/ui/section-card.tsx's HEADER_CLASS = 'border-b p-4' gives the header uniform padding on all sides — tighten specifically the space between the title/subtitle and the border-b beneath it (e.g. px-4 pt-4 pb-3, adjust to taste). This is a shared component, so it affects every SectionCard on every page — please eyeball a couple of other pages that use it (e.g. /manage/applications/[id]) to confirm it still looks right there too, not just on this edit page.

4. Cut the Applications section entirely. Remove the "Applications" SectionCard from the edit page — the PositionApplicationsSummary import/usage, the counts/total plumbing that only fed it, and the "Review applications" link that lived on that card's header. Check getPositionApplicationStats is still needed for anything else on this page (it also feeds unresolvedTotal, used by the archived-callout and PositionStatusHeaderActions — keep whatever's still load-bearing, drop the rest).

5. Danger zone cleanup. In components/features/position-danger-zone.tsx:

  • Move the "Delete position" button to the right, inline with the description text on the same row (not its own row below).
  • The blocked-reason text ("This position has N applications, so it can't be deleted...") currently reads as a second, separate line of info text rather than a warning. Instead of appending it below the normal description, have it replace the normal description text when blocked is true — one line of text that changes based on state, not two stacked lines.

Please investigate the current code yourself (branch 666-redesign-the-edit-position-page) rather than assuming file contents from this description, and update docs/DESIGN.md/docs/WORKFLOWS.md if either documents the removed Applications section or the old two-card Details/Availability split.

@b-at-neu b-at-neu added revising Revise agent working (in-flight) and removed needs revision Review found issues that need fixing labels Sep 8, 2026
@b-at-neu
b-at-neu force-pushed the 666-redesign-the-edit-position-page branch from 5964daf to 8b4fdbb Compare September 8, 2026 02:47
@b-at-neu

b-at-neu commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Revision — human layout feedback

merged Availability into Details (title → opens/closes → description), Managers/Questions now side by side, dropped the Applications summary card, tightened SectionCard header padding (px-4 pt-4 pb-3), inlined the danger-zone delete button with state-dependent description text · 8b4fdbb

@b-at-neu b-at-neu added ready for review PR ready for review agent and removed revising Revise agent working (in-flight) labels Sep 8, 2026
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Human feedback — post-approval revision

  1. SectionCard header padding still isn't right. The space below the title is still visibly larger than the space above it. Make the header's top and bottom padding equal in components/ui/section-card.tsx's HEADER_CLASS (currently border-b px-4 pt-3 pb-2 — asymmetric). Set it to border-b px-4 py-3 so top and bottom match exactly.
  2. Rename the "Reopen" button to "Reopen position" in lib/constants.ts (currently label: 'Reopen' in the closed→open transition action), matching the "Close position"/"Open position" naming already used for the other transition buttons. Leave confirmLabel: 'Reopen position' as is — it already matches.

b-at-neu and others added 9 commits September 12, 2026 00:12
Replaces the three-tab edit position page with a single max-w-5xl
scroll of SectionCards: status moves out of the form and into header
actions (mirroring the application detail page), and every field
autosaves on blur through four narrow server actions instead of one
whole-form submit. Also fixes managers staying editable on an archived
position.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rebase onto dev to fix conflicting merge state and unblock CI; switch
field-save actions to updateMany + count fallback for the stale-link
race, and trim over-length comments per ENGINEERING §7.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merge Availability into the Details card (title, then opens/closes
side by side, then description), put Managers and Questions side by
side, drop the Applications summary section, tighten SectionCard's
header padding, and inline the danger zone's delete button with
state-dependent description text.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The loading.tsx still modeled the old 5-card layout after the page was
merged into a Details+Availability card, a Managers/Questions grid,
and no Applications card — rebuilt it to match. Also picks up the
SectionCard `icon` prop that became required on `dev` since this
branch diverged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per the human decision on the R5-M1 escalation: closed -> draft is no
longer a legal position transition (independent of closesAt or
application count), and Reopen is now always rendered for a closed
position, disabled with a keyboard-reachable tooltip explanation
instead of disappearing. Also trims the R5-L1 stale comment on
APPLICATION_PIPELINE_STATUSES.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tighten SectionCard header padding site-wide, rename the draft/open and
open/closed header buttons to Open position/Close position, extend the
always-rendered-disabled-with-tooltip pattern to Open position for a
manager, and add a full-width live-edit warning bar for open/closed
positions.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Archived positions are always closed, so a non-admin manager saw the
live-edit warning ("changes are immediately visible") directly above
the archived callout saying the opposite. Gate on canEdit to match
the header actions' existing gate.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Addressed the feedback from #690 (comment):

  1. SectionCard's HEADER_CLASS in components/ui/section-card.tsx is now border-b px-4 py-3 (equal top/bottom padding), replacing the asymmetric pt-3 pb-2.
  2. The closed→open transition's label in lib/constants.ts is now Reopen position, matching the Close position/Open position naming (its confirmLabel was already correct).

Verified prettier, eslint, and tsc all pass; unit tests pass (410/410; the one pre-existing DB-dependent test file is unrelated and requires local Postgres, not part of this fix). Pushed as ab3d8f2.

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Code Review — Cycle 9 · needs revision

1 open — 1 🟠 Medium (see inline)

Comment thread lib/constants.ts
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Escalating to needs human

PR #690 (#666 Redesign The Edit Position Page) has hit 9 review cycles, and Cycle 9 still produced a Medium finding — the cycle cap stops autonomous revision again.

Cycle 9 finding

R9-M1 (Medium) — the "Reopen" → "Reopen position" button rename (from the last round of feedback) is consistent in code (button, confirm dialog, tooltip all agree), but docs/WORKFLOWS.md (the header-actions paragraph and two closed-transition bullets, ~lines 423/433/440) and docs/PERMISSIONS.md (the transition table, ~line 98) still say plain "Reopen" — docs and shipped UI text now disagree.

Suggested fix: update those doc references from "Reopen" to "Reopen position" to match the code.

Why this is escalating despite being trivial

Same as the last two escalations on this PR — the cycle cap is a blanket safety valve, not a severity judgment. 9 rounds crosses the threshold regardless of how small the fix is. Say the word and I'll send it back for revision, bypassing the cap.

Full review: #690 (review)

@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Confirmed — apply the doc fix (update the stale "Reopen" references in docs/WORKFLOWS.md and docs/PERMISSIONS.md to "Reopen position"). Bypassing the cycle cap since this is a direct human sign-off, not another autonomous cycle.

Update docs/WORKFLOWS.md and docs/PERMISSIONS.md so the reopen
transition's documented label matches the shipped "Reopen position"
button/dialog/tooltip text.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Revision — Cycle 9

fixed R9-M1 · 479b7aa

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Code Review — Cycle 10 · needs revision

1 open — 1 🟠 Medium (see inline)

Comment thread docs/WORKFLOWS.md Outdated
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Escalating to needs human

PR #690 has hit 10 review cycles; Cycle 10 found one more Medium — the cycle cap stops autonomous revision again.

Cycle 10 finding

R10-M1 (Medium)docs/WORKFLOWS.md:429 still reads "the header's Open position/Reopen button". The R9-M1 fix renamed this everywhere else (docs/WORKFLOWS.md lines 423, 433, 440; docs/PERMISSIONS.md line 98; lib/constants.ts line 878), but this one bullet — added in an earlier commit and never touched by the rename commits — was missed.

Suggested fix: same as R9-M1, just this one remaining line: change "Reopen" to "Reopen position" at docs/WORKFLOWS.md:429.

Why this is escalating

Same blanket cycle-cap rule as the last two rounds on this PR — not a severity judgment. Say the word to bypass, same as before.

Full review: (posted inline on docs/WORKFLOWS.md:429)

@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Confirmed — apply the fix (change "Reopen" to "Reopen position" at docs/WORKFLOWS.md:429). Bypassing the cycle cap since this is a direct human sign-off, not another autonomous cycle.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Revision — Cycle 10

fixed R10-M1 · ab1b5b4

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Code Review — Cycle 11 · approved

0 open — R10-M1's fix (ab1b5b4, single-line WORKFLOWS.md wording) verified correct; no regressions in this revision's delta; all required CI checks (prettier, lint, tsc, test, build) pass.

@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Root cause found (verified in the live Vercel preview via DOM inspection)

The padding-underneath-title issue was never actually fixed by cycles 6/9/10 — I checked the rendered page (/manage/positions/bypass-position/edit on this PR's preview) and measured computed styles directly:

headerPaddingTop: 12px
headerPaddingBottom: 24px   <- still 2x the top, despite HEADER_CLASS = 'border-b px-4 py-3'

Root cause: components/ui/card.tsx's base CardHeader primitive ships [.border-b]:pb-6 baked into its default className — an arbitrary-variant rule that forces 24px bottom padding on any CardHeader carrying border-b, which SectionCard always does. This rule wins the CSS cascade over the plain py-3/pb-3 utility SectionCard passes in (tailwind-merge doesn't dedupe an arbitrary-variant selector against a plain utility, so both ship, and the arbitrary variant's specificity wins). Every previous fix changed the losing side of that fight, so nothing visually changed no matter what value was tried.

Fix: in components/ui/section-card.tsx, use the Tailwind v4 important-suffix so the explicit value actually wins: change HEADER_CLASS from 'border-b px-4 py-3' to 'border-b px-4 py-3 pb-3!' (the trailing pb-3! beats [.border-b]:pb-6 in the cascade). Verify in the live preview afterward, not just by reading the class string — that's how the first three attempts slipped through.

CardHeader ships `[.border-b]:pb-6` in its own default className, which
beats the plain `py-3`/`pb-3` utility SectionCard passed via cn() —
tailwind-merge doesn't dedupe an arbitrary-variant selector against a
plain utility, so both classes ship and the arbitrary variant wins the
cascade. Prior cycles (6/9/10) only ever edited the losing side.
HEADER_CLASS now appends `pb-3!` (Tailwind v4 important suffix), which
compiles to `!important` and unconditionally beats the non-important
`[.border-b]:pb-6` rule regardless of selector specificity. Verified by
compiling both classes through the actual @tailwindcss/postcss engine.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Revision — root-cause fix for header padding (re: this comment)

Confirmed the diagnosis and applied the suggested fix: HEADER_CLASS in components/ui/section-card.tsx is now 'border-b px-4 py-3 pb-3!' (ec04328).

Why cycles 6/9/10 never changed anything, and why this one does:
components/ui/card.tsx's CardHeader ships [.border-b]:pb-6 in its own default className. cn() here is twMerge(clsx(inputs)), and I confirmed directly with the installed tailwind-merge package that it does not dedupe an arbitrary-variant selector ([.border-b]:pb-6) against a plain pb-* utility — both classes always shipped in the final class list, regardless of what value SectionCard passed. Every prior fix only ever edited the losing side of that fight.

I didn't stop at reasoning about class strings — I compiled both classes through the actual @tailwindcss/postcss engine (v4.3.1, the version this repo uses) to see the real generated CSS:

.pb-3\! {
  padding-bottom: calc(var(--spacing) * 3) !important;
}
.\[\.border-b\]\:pb-6 {
  &:is(.border-b) {
    padding-bottom: calc(var(--spacing) * 6);
  }
}

pb-3! compiles to !important; [.border-b]:pb-6 does not (no global important flag is set anywhere in this repo's Tailwind config either). Per the CSS cascade, an !important declaration always wins over a non-!important one for the same property on the same element, independent of selector specificity or source order — so pb-3! unconditionally beats [.border-b]:pb-6 in every browser. High confidence this is the definitive fix, verified against the compiled CSS rather than just the class string.

Scope: only section-card.tsx changed, per the instruction not to touch the shared card.tsx primitive — other CardHeader consumers keep their existing pb-6 behavior; only SectionCard (and its skeleton) get the tightened 12px bottom padding, applied uniformly to every SectionCard screen (dashboards, /applications/[id], /manage/applications/[id], etc.) as previously flagged in R3-L1, which remains open pending a visual pass across those other screens.

Verification: prettier:check, eslint:check, tsc:check all pass; npm run test:unit — 410/410 passing (the one DB-backed suite fails locally only because DATABASE_URL isn't set in this worktree, unrelated to this change).

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Code Review — Cycle 12 · approved

1 open — 0 Critical, 0 Medium, 1 Low (see inline)

Comment thread components/ui/section-card.tsx Outdated
import { Skeleton } from '@/components/ui/skeleton';

const HEADER_CLASS = 'border-b p-4';
const HEADER_CLASS = 'border-b px-4 py-3 pb-3!';

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

R12-L1 🟡 Low — pb-3! forces !important to beat CardHeader's own [.border-b]:pb-6; verified working, but it's a blunt, permanent override on a primitive every page consumes — a future consumer that legitimately wants taller header padding on a border-b SectionCard is now defeated regardless of specificity. Consider matching the default's selector shape (e.g. an arbitrary variant of the same compound specificity) instead of reaching for !important, so the override stays contestable.

@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Second padding source found (measured via computed layout on the live preview)

The pb-3! fix is real and working — measured paddingBottom: 12px now, matching paddingTop: 12px. But there's a second, unrelated source of extra space below the title that has nothing to do with padding:

components/ui/card.tsx's base CardHeader sets grid-rows-[auto_auto] (an explicit 2-row grid template, for the optional CardDescription-below-title case) plus gap-1.5 (6px row-gap). SectionCard never renders a second grid row/child inside CardHeader — its title+icon content is a single wrapping &lt;div&gt;, so all content lands in grid row 1. But CSS Grid still reserves the second explicit row track (even though it's empty, height 0) and still renders the 6px row-gap after row 1, since gap applies between defined tracks regardless of whether they contain content.

Measured: gridTemplateRows: 24px 0px, and the actual gap from the title's bottom edge to where content starts is 19px (12px padding + ~6px phantom row-gap + rounding), while the top only has the 12px padding with no equivalent extra row/gap before it — hence "still not as little as on top" even after the pb-3! fix.

Fix: in components/ui/section-card.tsx, add grid-rows-1 to HEADER_CLASS (a plain utility, not fighting an arbitrary variant this time — tailwind-merge correctly dedupes plain grid-rows-* utilities against each other, so this one should just work without needing !important). This collapses the header to a single grid row, eliminating the phantom row and its gap. Resulting HEADER_CLASS: 'border-b px-4 py-3 pb-3! grid-rows-1'.

Verify by measuring computed layout again after the change (or reasoning precisely through the grid math), not just by reading the class string.

CardHeader's grid-rows-[auto_auto] reserved a second, empty row plus
its 6px gap-1.5 row-gap below the title even with no second grid
child, so the header stayed taller on the bottom than the top after
the pb-3! padding fix. grid-rows-1 removes the phantom track and gap.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@b-at-neu

Copy link
Copy Markdown
Collaborator Author

Follow-up fix — phantom grid row-gap

Added grid-rows-1 to SectionCard's shared HEADER_CLASS (components/ui/section-card.tsx), used by both SectionCard and SectionCardSkeleton. Result: border-b px-4 py-3 pb-3! grid-rows-1.

Verification (two-step, on the live Vercel preview for this push).

  1. Ran HEADER_CLASS through tailwind-merge (the same cn() used at render): grid-rows-[auto_auto] is dropped entirely (same conflict group as grid-rows-1, later class wins — plain-utility dedup, no !important fight needed).
  2. Measured getComputedStyle on /manage/positions/<id>/edit's "Details" CardHeader after deploy: gridTemplateRows is now "24px" (was "24px 0px"), and title-to-header-edge spacing is 12px above / 13px below (was 12px / 19px) — the 1px residual is the border-b divider line, not a phantom row. Same result on the dashboard's SectionCards.

Confidence: high. Directly measured on the deployed preview post-fix, not inferred from the class string.

5dead6e

@b-at-neu b-at-neu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Code Review — Cycle 13 · approved

0 open — this revision is a single one-line commit (5dead6e, grid-rows-1 on SectionCard's HEADER_CLASS) closing the phantom second grid row left by the R12-L1 pb-3! padding fix; verified correct (CardHeader's single flex-div child never needed the second auto track), no regressions, all required CI checks pass.

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

Labels

approved Review passed, ready to merge claude Will be worked on by Claude

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redesign The Edit Position Page

1 participant