#666 Redesign The Edit Position Page - #690
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
b-at-neu
left a comment
There was a problem hiding this comment.
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.
a4e6137 to
5964daf
Compare
Revision — Cycle 1fixed 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
left a comment
There was a problem hiding this comment.
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.
Human feedback on the redesigned edit-position pageFive changes, in order of the page layout: 1. Merge Availability into Details. Combine the "Availability" 2. More side-by-side, less vertical stacking. Two concrete opportunities:
3. Decrease the padding underneath card titles. 4. Cut the Applications section entirely. Remove the "Applications" 5. Danger zone cleanup. In
Please investigate the current code yourself (branch |
5964daf to
8b4fdbb
Compare
Revision — human layout feedbackmerged Availability into Details (title → opens/closes → description), Managers/Questions now side by side, dropped the Applications summary card, tightened |
Human feedback — post-approval revision
|
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>
|
Addressed the feedback from #690 (comment):
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
left a comment
There was a problem hiding this comment.
Code Review — Cycle 9 · needs revision
1 open — 1 🟠 Medium (see inline)
Escalating to
|
|
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>
Revision — Cycle 9fixed R9-M1 · 479b7aa |
b-at-neu
left a comment
There was a problem hiding this comment.
Code Review — Cycle 10 · needs revision
1 open — 1 🟠 Medium (see inline)
Escalating to
|
|
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>
Revision — Cycle 10fixed R10-M1 · ab1b5b4 |
b-at-neu
left a comment
There was a problem hiding this comment.
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.
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 ( Root cause: Fix: in |
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>
Revision — root-cause fix for header padding (re: this comment)Confirmed the diagnosis and applied the suggested fix: Why cycles 6/9/10 never changed anything, and why this one does: I didn't stop at reasoning about class strings — I compiled both classes through the actual .pb-3\! {
padding-bottom: calc(var(--spacing) * 3) !important;
}
.\[\.border-b\]\:pb-6 {
&:is(.border-b) {
padding-bottom: calc(var(--spacing) * 6);
}
}
Scope: only Verification: |
b-at-neu
left a comment
There was a problem hiding this comment.
Code Review — Cycle 12 · approved
1 open — 0 Critical, 0 Medium, 1 Low (see inline)
| import { Skeleton } from '@/components/ui/skeleton'; | ||
|
|
||
| const HEADER_CLASS = 'border-b p-4'; | ||
| const HEADER_CLASS = 'border-b px-4 py-3 pb-3!'; |
There was a problem hiding this comment.
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.
Second padding source found (measured via computed layout on the live preview)The
Measured: Fix: in 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>
Follow-up fix — phantom grid row-gapAdded Verification (two-step, on the live Vercel preview for this push).
Confidence: high. Directly measured on the deployed preview post-fix, not inferred from the class string. |
b-at-neu
left a comment
There was a problem hiding this comment.
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.
Closes #666
Summary
/manage/positions/[id]/editas onemax-w-5xlscrolling page ofSectionCards (a merged Details+Availability card, Managers, Questions, Danger zone) — no tabs.position-applications-summary.tsxwas 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.PageHeaderheader actions (a split button + overflow, mirroring/manage/applications/[id]'s status control), so illegal transitions are simply never offered instead of reachable-then-refused.updatePositionsubmit 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.PositionManagersReadonlyview), and warning copy referenced field position ("below") rather than the field itself.draft → openandopen → closedheader buttons are renamed Open position and Close position (were Publish and Close applications —closed → 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 foropen/closedpositions.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 foropen/closedpositions.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.tsxwas added then deleted — see Scope change above.)prisma/actions/position-actions.ts—updatePositionreplaced by the four field actions sharingauthorizePositionEdit/revalidatePositionSurfaceshelpers;addPositionManager/removePositionManagernow also gate oncheckPositionEditable.lib/constants.ts— splitpositionFormShapeinto reusable per-field schemas; addedgetPositionTransitionTargets,POSITION_TRANSITION_ACTIONS,POSITION_OPEN_REQUIRES_ADMIN_NOTE(renamed fromPOSITION_PUBLISH_REQUIRES_ADMIN_NOTE),POSITION_REOPEN_REQUIRES_ADMIN_NOTE,POSITION_CLOSED_DRAFT_BLOCKED_ERROR,POSITION_LIVE_EDIT_WARNING; movedPIPELINE_STATUSESto the sharedAPPLICATION_PIPELINE_STATUSES; removedclosed → draftfromPOSITION_STATUS_TRANSITIONS; renamed thedraft → open/open → closedaction labels to Open position / Close position; removed the now-unreachablePOSITION_UNPUBLISH_BLOCKED_HINTnote branch.components/features/markdown-field.tsx—disabled/footer/onCommitprops, minimal{ description }context type.components/ui/section-card.tsx— new'form-field'skeleton row shape; tightened header padding (pt-4 pb-3→pt-3 pb-2) site-wide, applying to everySectionCardconsumer.position-edit-tabs.tsx,position-details-form.tsx,position-details-readonly.tsx.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/unit/position-transitions.test.tsaroundgetPositionTransitionTargets; addedPOSITION_TRANSITION_ACTIONS/schedule-validation cases totests/unit/constants.test.ts; migratedtests/db/position-transitions.test.ts/position-publish-permission.test.tstoupdatePositionStatus; addedtests/db/position-field-saves.test.ts; added archived-manager cases totests/db/position-archive.test.ts.Testing plan
opensAtis in the past → only the changed field is checked.closedposition 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.closedposition: Reopen is shown disabled with a tooltip explaining only an admin can reopen it; there is no Return to draft option at all on aclosedposition, for either role.openposition with applications: Return to draft is absent; Close position confirms with the unresolved count.addPositionManager/removePositionManageron an archived position →ARCHIVED_POSITION_EDIT_ERROR; an admin's succeeds.draftposition → no live-edit warning bar. Open anopenorclosedposition → the warning bar appears under the page title, reading "Changes made here are immediately visible in the live application."/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— passnpm run eslint:check— passnpm run tsc:check— passnpm run test:unit— pass (376/376;tests/unit/email-delivery-events.test.tsfails in this sandbox only becauseDATABASE_URLisn't set locally — confirmed pre-existing ondev, 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 unmodifieddevtests/db/*— not run locally (no Postgres available in this environment); will run in CINotes
PositionManagersSectioncapability reversal (managers no longer editable on an archived position) is a deliberate policy change called out in the plan's risks —docs/PERMISSIONS.mdis updated accordingly.closedposition 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 forclosesAtPast).closed → draftis no longer a legal transition at all (POSITION_STATUS_TRANSITIONS), independent ofclosesAtor application count — reopening is the only way out ofclosed. Reopen is now always rendered for aclosedposition rather than conditionally hidden — it rendersdisabledwith a keyboard-reachable tooltip explanation (admin-only, or past close date) instead of disappearing.docs/PERMISSIONS.mdanddocs/WORKFLOWS.mdare updated accordingly; see the PR discussion for the full decision.SectionCard's shared header padding, resolving the earlier-deferred R3-L1 finding; (2)/(3) renamed theopen/closeheader 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 foropen/closedpositions.docs/WORKFLOWS.mdanddocs/PERMISSIONS.mdare updated accordingly.