fix(export): certifications drop type picker, handle no-year, compress onto one PDF line (#899) - #904
Conversation
Deploying offlinecv with
|
| Latest commit: |
05e34e4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0334a4e8.offlinecv.pages.dev |
| Branch Preview URL: | https://gh-899-certifications-compac.offlinecv.pages.dev |
rohithgollapalli
left a comment
rohithgollapalli
left a comment
There was a problem hiding this comment.
Read #899's acceptance criteria first, then the diff, then the description. This is careful work — the run-based collapse in achievements.ts, the atomic-wrap reuse, and the foldCertificationTypes seam are all the right shape, and the round-trip test comparing entries rather than counts is what makes the compaction claim mean something. One finding blocks, and it is a capability the compact line removes rather than a bug in it.
Blocking
1. A dateless certification on the compact line can no longer be given a year — AC 5 of #899, under Closes #899. AchievementYearSlot returns null when compact && !year (ReconstructedResume.tsx:1030), so a parsed credential the résumé never dated draws no year field and no add-affordance. The PR's own test pins it ("offers no '+ year' affordance on a dateless compact credential"), and the docblock calls the cost "real and deliberate" — so this is a decision, not an oversight. But #899 AC 5 asks that "each certification in the compact UI remains individually editable (title, year) and removable", and the user-visible effect is a regression against main, where every certification row carried a year field: a user who drops a résumé listing three undated credentials and wants to date them before exporting now has no path to it short of removing each one and re-adding it as a user-added credential (which takes the full-width branch and keeps the ordinary slot).
The gap is also the one thing the description's own "Known, disclosed, non-blocking gap" section does not cover — it discloses the UI/PDF predicate divergence, not this.
Three ways out, any of which clears it: render the affordance and accept the clutter; reveal it on row hover/focus so it costs nothing at rest; or keep the tradeoff and amend AC 5 on #899 plus say so in the PR body, so the issue does not close with a criterion silently unmet.
Secondary
2. An undated "Credential · Issuer" line now splits into two certifications, the second named after the issuer. isDatedCredentialRow requires three or more segments (achievements.ts:339), so the two-segment undated shape falls through to the per-credential split — achievements.test.ts:477 pins "AWS · Certified Solutions Architect" → [{title: "AWS"}, {title: "Certified Solutions Architect"}]. The reasoning in that test is sound and I am not asking you to reverse it: the other reading merges every credential of our own exported PDF into one and loses N-1 outright, which is worse. But it is the same class as the fabrication rounds 1-3 fixed, it fires on a common source shape, and unlike the divergence in the description's known-gap section it is not disclosed anywhere the reader of the PR will see it. Either narrow it (a two-segment run in a section whose other lines carry no separator and no dated run is much more likely a source quirk than our own output) or promote it into the known-gap section so it is a stated tradeoff rather than a discovery.
3. render-ats-pdf.ts:2090's "SAME label" claim is now false for a compacted section. collectModelTextFields (render-findings.ts:180-186) walks section.entries and labels each credential individually; the renderer draws one synthesized entry and labels it entryPathLabel(heading, drawn[0], 0), whose headerLine is the whole joined line. So a pagination finding on a compacted Certifications block reports Certifications → AWS Certified Solutions Architect (2022) · CKA · Terr… while a glyph finding about the same credential reports Certifications → CKA — exactly the drift that comment says cannot happen. compactLine is also never walked by collectModelTextFields, so the drawn separator is outside the glyph audit (low risk in practice — · is already audited via the skills list). Route the label off section.entries[i] where a compact section is drawn, or correct the comment.
Nits
4. The compact container is applied to every certifications section, even one where nothing joins the line. const compact = section === "certifications" (ReconstructedResume.tsx:1161) swaps flex flex-col gap-4 for flex flex-wrap … gap-y-2 unconditionally, so an all-bulleted certifications block (the latex/awesome-cv-cv.pdf shape) still renders as full-width w-full rows but at half the vertical gap achievements get. Harmless, probably unintended. Deriving the container from sharesCompactLine instead would keep that case byte-identical to main.
5. fallow attributes 9 complexity findings to this diff — buildAtsResumeModel (32 cyclomatic / 526 lines), buildHeuristicResult (37/210), pushAddedEntry (13/17/66). Report-only per CLAUDE.md and not a merge concern; noting it because the PR grows two functions that were already over the line rather than extracting.
6. A test comment overstates what it asserts — see the inline suggestion on ReconstructedResume.remove-parsed-entry.test.tsx:472.
Gates
| Gate | Result |
|---|---|
/code-review correctness pass |
folded in above |
| AC checklist (#899) | 5 of 6 met; AC 5 partially unmet (finding 1). AC 1-4 and 6 verified against the code and the new tests |
| 3a fixture PII | skipped — no fixtures touched (git diff --name-only has nothing under tests/) |
| 3b design-system / reuse | pass — no raw <button>/<input>/<dialog> added, no new component files |
| 3c style tokens | pass — the only hex-shaped matches are #899/#884/#380/#301 issue refs |
| 3d fallow | pass as a gate; 9 complexity findings, 28 clone groups, report-only (finding 5) |
| 3e command-level | skipped — no skill/script/bash files in the diff |
| 3f description accuracy | accurate, and unusually forthcoming — the three adversarial rounds and the UI/PDF predicate divergence are all real and all check out against the code. Omits findings 1 and 2 |
typecheck / lint |
green |
| touched suites | 268 tests green (achievements, ats-resume-model, apply-overrides, ReconstructedResume.remove-parsed-entry, corpus-roundtrip — including the three-credential compaction hop) |
| full suite | 6357 passed / 4 failed — all four are jd-extract/schema-org* salary formatting (expected 'USD 1,50,000+' to be 'USD 150,000+'), an unpinned Intl locale on this reviewer's machine. Independent of this diff (src/lib/jd-extract/ imports none of the changed modules) and not attributed to this PR; the description's "0 failed" is consistent with a machine whose default locale groups Western-style |
Verdict rule: ≥1 Blocking finding → REQUEST_CHANGES. Nothing was committed or pushed; the branch is already one commit, so the invariant is intact.
Reviewed by: Claude Opus 5 (high)
| * Fabricating a credential on the first parse is the worse of the two. | ||
| */ | ||
| function isDatedCredentialRow(run: string[]): boolean { | ||
| if (run.length < 3) return false; |
There was a problem hiding this comment.
Secondary — the two-segment undated shape still splits into two credentials. Requiring three segments means "AWS · Certified Solutions Architect" falls through to the per-credential split and yields [{title: "AWS"}, {title: "Certified Solutions Architect"}], which achievements.test.ts:477 pins deliberately. I am not asking you to flip that: the opposite reading merges every credential of our own exported PDF into one and loses N-1, which is plainly worse, and the test says so.
Two things about it are still worth acting on. It is the same class as the fabrications rounds 1-3 fixed — a credential the résumé never claimed, on a hiring document — and "Credential · Issuer" is an ordinary way to write a certifications line, so this fires on real dropped résumés rather than on a contrived one. And unlike the UI/PDF predicate divergence, it is disclosed only in a test comment, not in the PR body or on #899.
If a narrowing is available, the section-level signal is the one to use: a two-segment run inside a section whose other lines carry no separator and no date-terminated run looks far more like a source quirk than like our own compact output, which always emits parenthesised years and two or more credential-shaped segments. If not, promoting it into the "Known, disclosed" section is enough — it should be a stated tradeoff, not something the next reader discovers from a parse.
There was a problem hiding this comment.
Not fixed — disclosed instead, per the fallback you offered. Evaluated the section-level narrowing first: it would need appendCredentialLine/dateTerminatedRuns to see whether a section's OTHER lines carry separators/dated runs, which they currently don't (they operate per-line, not with section context threaded in) — plumbing that through is a real change to the extraction call chain, not a narrow fix, so I didn't attempt it under Secondary severity this late in review. Promoted it into the PR's "Known, disclosed, non-blocking gap" section instead, naming the exact shape ("Credential · Issuer", no date, 2 segments) and the tradeoff against the 3+-segment collapse. Leaving this open — happy to pick the narrowing up as a follow-up if you'd rather it not ship as a disclosed gap.
af1ff52 to
bfffb78
Compare
…s onto one PDF line (#899) Certifications (#884) shared achievements' type-splitting and type-picker UI, which is wrong for name-led credentials. This preserves the full credential name instead of splitting off a bogus "type", stops a dateless certification from rendering a dangling separator (in both the UI and the export), and compresses 2+ certifications onto one compact, middot-delimited line in the ReconstructedResume UI and the exported PDF using the existing atomic-wrap segment machinery, so the round trip (parse -> export -> re-parse) reproduces the original entries. A run-based collapse (isDatedCredentialRow / dateTerminatedRuns) recognizes a "Name . Issuer . Year" source line as one dated credential instead of splitting it into two, so a certification named after its own issuing organization is never fabricated with an invented year. The collapse is judged per date-terminated run rather than per line, so a single physical line holding two dated credential triples (e.g. a two-column block merged by line assembly) still collapses each of them correctly, and a credential whose own title contains the list separator is excluded from the compact line so a second export hop can't re-fabricate a dated entry from it. A dateless credential on the compact line keeps a "+ year" add-affordance (hidden at rest, revealed on the row's hover/focus) rather than losing it outright, so a parsed-but-undated credential stays dateable before export. The compact flex-wrap container only applies when a section's line is genuinely shared, not to every certifications section, so an all-bulleted block keeps the ordinary row spacing. The PDF glyph audit now also walks a compacted section's synthesized line directly, since it's the one string the page actually draws that the per-credential entries walk doesn't otherwise see. Closes #899
bfffb78 to
05e34e4
Compare
|
@rohithgollapalli — addressed all five threads from your review, rebased onto Fixed (4):
Disclosed, not fixed (1):
One thing outside this PR's scope, found afterward: the Stop hook's post-push Gates: Re-requested your review — this should be ready for a fresh approval. |
|
Merging this now rather than holding it — I'm going to be away from the computer and don't want it sitting open while I'm offline. Functionality is verified through the UI. @rohithgollapalli — I'm merging ahead of clearing your review comments from 8/26, so they're not lost, just not blocking. Happy to take anything still outstanding as a follow-up. |
The Blocking AC-5 finding was addressed in 05e34e4 (force-pushed 2026-08-27): AchievementYearSlot now reveals the "+ year" affordance on row hover/focus for a dateless compact credential — the second of the three remedies offered — so a parsed undated credential can be dated again without removing and re-adding it. Secondary 3 and the nits are resolved. Secondary 2 (the two-segment "Credential · Issuer" split) is intentionally left open and answered in-thread: the section-level narrowing needs section context threaded through the extraction call chain, so it was promoted into the PR body as a disclosed tradeoff, with a follow-up offered. Per repo convention only Blocking findings block, and a reply is the exit. verify / fallow / Cloudflare Pages are green and offlinecv-approver has approved the current head. Dismissing so the merge queue is not held by a review written against the pre-revision diff.
3 participants
Summary
Certifications (#884) shared achievements' type-splitting and type-picker UI, which is wrong for name-led credentials. This fixes three bugs surfaced by intern testing and adds a compact multi-credential display in both the UI and the exported PDF:
extractAchievementsno longer runssplitAchievementTypeon certification titles — the full credential name is preserved,typestays undefined.AchievementTypePickerand stray type labels no longer render on certification rows.MIDDOT_SEGMENT_SEP/wrapSegmentsToLines) so the round trip (parse → export → re-parse) reproduces the original entries.isDatedCredentialRow/dateTerminatedRuns) recognizes a "Name · Issuer · Year" source line as one dated credential instead of splitting it into two, so a certification named after its own issuing organization is never fabricated.Closes #899
Adversarial review
Three rounds, pre-push, on the full branch diff — this touches correctness-critical extraction logic on a hiring document, so the bar was high.
Round 1 — 5 blocking findings, all fixed:
"CKA · May 2021") was silently dropped — the rejoin predicate didn't admit month-year.·glyph for both the year separator and the item boundary — ambiguous.typefield (from résumés saved before this change) became invisible, uneditable, and broke the compaction guard — fixed with a fold-back seam inapply-overrides.ts."Name · Issuer · Year"produced two entries, one a certification named after the issuer with an invented year.Round 2 — 1 blocking finding, fixed: the round-1 fabrication guard was per-line, not per-run, so it still fabricated when a single physical line held two dated credential triples (e.g. a two-column certifications block merged by line assembly). Fixed by evaluating the collapse per date-terminated run instead of per line.
Round 3 (final verification) — 1 blocking finding, fixed: the run-collapse could itself produce a credential title containing the list separator (e.g.
"Google Cloud Architect · Google"), and compacting that onto a shared PDF line put an unescaped separator inside one credential — the second export hop (download → re-upload) then re-fabricated a dated entry from the trailing segment. Fixed by extending the existing compaction guard to also refuse when a credential's own title carries the separator; verified end-to-end (parse → export → re-parse) that no fabrication survives on either hop.All rounds also surfaced and fixed several nits (an added entry's legacy
typelabel wasn't folded; two docblocks overstated cross-surface parity between the UI and PDF compaction predicates — both corrected to disclose the real, content-preserving divergence rather than claim exact parity).Review round (PR feedback)
main, where every certification row carried a year slot) — it now rendersopacity-0at rest and reveals on the row's hover/focus, so a parsed-but-undated credential stays dateable before export ([export] Certifications: drop stray type-picker dropdown, handle no-year, compress multiple onto one line #899 AC 5) while the shared line stays uncluttered at rest.gap-y-2) now applies only when the section's line is genuinely shared (sharesCompactLine), not to every certifications section — an all-bulleted block keeps the ordinary vertical rhythm other full-width sections get.collectModelTextFields) now also walks a compacted section'scompactLinestring directly, since it's the one string the page actually draws that the per-credentialentrieswalk doesn't cover; theentryPathlabel-parity comment at the pagination call site andsectionDrawEntries' docblock now disclose the compacted-section exception instead of claiming unconditional parity."AWS · Certified Solutions Architect") still splits into two entries — a real but narrower-scoped version of the fabrication class rounds 1–3 fixed.Validation
npm run typecheck,npm run lint, and the full test suite (OFFLINECV_FULL_TESTS=1) — 379 files / 6361 tests passed, 0 failed.npm run check:fixturesclean (no fixtures touched). Pre-pushverifypassed (fallow complexity findings are report-only per repo convention).Known, disclosed, non-blocking gaps
"Credential · Issuer", no date) still falls through the run-based collapse (which requires 3+ segments) and splits into two entries —achievements.test.ts:477pins this deliberately, since the opposite reading (merge every 2-segment run) would collapse an unrelated multi-credential list into one entry and lose N-1 credentials, which is worse. This fires on a real, ordinary "Credential · Issuer" source line, not only a contrived one; a section-level signal (whether the section's OTHER lines carry separators/dated runs at all) could narrow it further but wasn't attempted here. Tracked as a known tradeoff, not silently discovered from the parser.