feat: rename usage scenario to requirement package - #177
Merged
Conversation
…eScenarioIds to requirementPackageIds in integration contracts
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR replaces "usage scenarios" with "requirement packages" across the codebase: DB schema, TypeORM entities, DAL, API routes, services, AI taxonomy/prompt, UI components, seed data, localization, docs, and tests. It adds requirement-package CRUD and join tables, removes scenario artifacts, and updates wiring, shapes, and fixtures. ChangesRequirement Packages Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Server/API
participant Service
participant DAL
participant DB
Client->>API: POST /api/requirements { requirementPackageIds, ... }
API->>Service: manageRequirement(payload)
Service->>DAL: createRequirement(requirementPackageIds,...)
DAL->>DB: INSERT requirement + INSERT into requirement_version_requirement_packages
DB-->>DAL: OK
DAL-->>Service: createdRequirement(with versionRequirementPackages)
Service-->>API: 201 Created { id, version }
API-->>Client: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/admin-center.md (1)
121-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the reference-data route list complete.
The new
requirement packagesentry is fine, but this summary now omits
norm referenceseven though that page is still part of the same admin
surface. Add it back so the list matches the actual reference-data routes.Suggested fix
- - areas (including owner assignment) - - types - - requirement packages - - statuses - - quality characteristics - - business objects - - implementation types + - areas (including owner assignment) + - types + - requirement packages + - norm references + - statuses + - quality characteristics + - business objects + - implementation types🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/admin-center.md` around lines 121 - 129, The reference-data route list in the admin-center documentation has omitted the "norm references" entry; update the list that currently includes "areas", "types", "requirement packages", "statuses", "quality characteristics", "business objects", and "implementation types" to also include "norm references" so the summary matches the actual reference-data routes (ensure the phrase "norm references" appears alongside the other bullet items in the same list).components/RequirementsTable.tsx (1)
2934-2982:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard requirement-package pills behind filter support.
Line 2934 renders clickable filter pills even when
onFilterChangeis absent; in that case clicks are no-ops becauseupdateFilterexits early. This exposes inert controls.💡 Suggested fix
- {requirementPackages.length > 0 && ( + {hasFilters && requirementPackages.length > 0 && ( <div className="flex items-center gap-2 border-b bg-white/80 px-3 py-2 text-sm backdrop-blur-sm dark:bg-secondary-900/80">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/RequirementsTable.tsx` around lines 2934 - 2982, The requirement-package pill UI is rendered and interactive even when filtering isn't supported (onFilterChange/updateFilter is absent), producing inert controls; update the render guard around requirementPackages (the whole block that maps requirementPackages and the clear button) to only render when updateFilter (or the prop/function that wraps onFilterChange) is present/allowed, or alternatively render non-interactive disabled pills when updateFilter is undefined; reference the requirementPackages map, the buttons that call updateFilter, and the (fv.requirementPackageIds ?? []) clear button so you hide or disable those controls when filtering support is not available.typeorm/seed.mjs (1)
11954-11986:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't swallow transaction cleanup failures.
A failed
commitTransaction()is currently ignored, soseedDatabase()can return success even when nothing was persisted. The rollback path also leaves the transaction flag set if rollback itself throws, which can lead to a bad commit attempt infinally.🔧 Proposed fix
} catch (error) { if (startedTransaction && runner) { - try { - await runner.rollbackTransaction() - startedTransaction = false - } catch { - // ignore rollback errors; original error is more important - } + try { + await runner.rollbackTransaction() + } finally { + startedTransaction = false + } } @@ } finally { if (startedTransaction && runner) { - try { - await runner.commitTransaction() - } catch { - // ignore commit errors here; if commit fails the caller's next - // operation will surface a clearer error - } + await runner.commitTransaction() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typeorm/seed.mjs` around lines 11954 - 11986, The catch/finally currently swallows transaction cleanup failures: ensure rollback failures clear startedTransaction and are propagated, and ensure commitTransaction failures in finally are not ignored. In the catch block (around runner.rollbackTransaction) set startedTransaction = false even if rollback throws and capture/rethrow the rollback error (or attach it to the original error) instead of silently ignoring it; in the finally block, when calling runner.commitTransaction(), do not swallow errors—catch to add context (including currentTable/currentRowIndex/currentRow) and rethrow so callers see that commit failed; still attempt runner.release() in a separate try/catch that can ignore release errors. Use the existing symbols runner, startedTransaction, commitTransaction, rollbackTransaction, currentTable, currentRowIndex, and currentRow to locate and change the logic.app/api/requirements/[id]/route.ts (1)
76-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
requirementPackageIdsbefore handing off to the service.The new array mapping still trusts the request body too much: duplicate or non-positive IDs will flow into the composite-PK join table and can turn a valid edit into a 500. Deduping and rejecting invalid IDs here would make the API more resilient to client mistakes.
🛠️ Suggested normalization
- requirementPackageIds: Array.isArray(body.requirementPackageIds) - ? body.requirementPackageIds - .map(value => Number(value)) - .filter(value => !Number.isNaN(value)) - : undefined, + requirementPackageIds: Array.isArray(body.requirementPackageIds) + ? [ + ...new Set( + body.requirementPackageIds + .map(value => Number(value)) + .filter(value => Number.isInteger(value) && value > 0), + ), + ] + : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/requirements/`[id]/route.ts around lines 76 - 89, The mapping for requirementPackageIds in app/api/requirements/[id]/route.ts should validate, dedupe and reject invalid IDs before calling the service: convert each value to Number, filter out NaN/non-positive (e.g., n => Number.isFinite(n) && n > 0), remove duplicates (e.g., via Set) and, if the resulting array is empty, set requirementPackageIds to undefined; apply the same stricter normalization pattern to normReferenceIds as needed so only unique, positive integers flow to the service.app/[locale]/requirement-packages/requirement-packages-client.tsx (1)
162-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't restore stale linked requirements on fetch failure.
previousLinkedRequirementscan re-populate the panel with rows from a different
package if the current fetch fails, which makes the error state misleading. Clear
the list on failure instead of reusing the prior package's data.♻️ Suggested fix
const fetchLinkedRequirements = useCallback( async (requirementPackageId: number) => { const requestId = ++linkedReqRequestId.current - const previousLinkedRequirements = linkedRequirements setLinkedRequirementsLoading(true) setLinkedRequirementsError(null) try { const response = await apiFetch( `/api/requirement-packages/${requirementPackageId}`, ) if (requestId !== linkedReqRequestId.current) return if (!response.ok) { - setLinkedRequirements(previousLinkedRequirements) + setLinkedRequirements([]) setLinkedRequirementsError(tc('error')) return } const data = (await response.json()) as { linkedRequirements?: LinkedRequirement[] } if (requestId !== linkedReqRequestId.current) return setLinkedRequirements(data.linkedRequirements ?? []) } catch { if (requestId === linkedReqRequestId.current) { - setLinkedRequirements(previousLinkedRequirements) + setLinkedRequirements([]) setLinkedRequirementsError(tc('error')) } } finally { if (requestId === linkedReqRequestId.current) { setLinkedRequirementsLoading(false) } } }, [linkedRequirements, tc], )Also applies to: 208-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[locale]/requirement-packages/requirement-packages-client.tsx around lines 162 - 193, The fetchLinkedRequirements callback currently saves previousLinkedRequirements and restores it when a fetch fails, which can show stale rows; change the error paths in fetchLinkedRequirements so that on non-ok responses and in the catch block you do not call setLinkedRequirements(previousLinkedRequirements) but instead clear the list (e.g., setLinkedRequirements([])) and setLinkedRequirementsError(tc('error')) — keep the requestId checks using linkedReqRequestId.current so updates only apply to the latest request and still clear/set linkedRequirementsLoading as before.
🧹 Nitpick comments (5)
tests/unit/version-detail-client.test.tsx (1)
23-52: ⚡ Quick winAdd one package to the fixture so the new path is exercised.
The mock now matches the new shape, but this test still only verifies the empty-array case. A populated requirement package would protect the rename from regressions.
Proposed test tweak
- versionRequirementPackages: [], + versionRequirementPackages: [ + { + requirementPackage: { + id: 7, + nameEn: 'Package', + nameSv: 'Kravpaket', + descriptionEn: null, + descriptionSv: null, + ownerId: null, + }, + }, + ],expect(screen.getByText('common.yes')).toBeInTheDocument() + expect(screen.getByText('Kravpaket')).toBeInTheDocument()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/version-detail-client.test.tsx` around lines 23 - 52, The test fixture returned by makeVersion currently sets versionRequirementPackages: [] which leaves the new code path untested; update makeVersion to include a single realistic package object in versionRequirementPackages (e.g., with id, name, and any fields used by the component under test) so the tests exercise the populated-package branch; ensure the object shape matches RequirementVersionDetail's expected package structure and leave overrides spread at the end to allow per-test customization.tests/integration/requirements-table-column-picker.spec.ts (1)
152-176: ⚡ Quick winPrefer a stable selector over seeded package text.
Line 152 binds to the exact label
'Mobil användning', which is brittle across fixture/content updates. Consider asserting via a dedicateddata-*marker on requirement-package filter pills instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/requirements-table-column-picker.spec.ts` around lines 152 - 176, Replace the brittle exact-text selector that defines requirementPackageFilter (currently using page.getByRole('button', { name: 'Mobil användning' })) with a stable data-* selector on the requirement-package filter pill (e.g., use a dedicated attribute like data-requirement-package or data-test-requirement-package) so the test queries by that attribute instead of seeded text; update any assertions that reference requirementPackageFilter to use the new selector and ensure the app fixtures add the corresponding data attribute to the requirement-package pill component.tests/unit/requirements-list-route.test.ts (1)
144-165: ⚡ Quick winAssert that
requirementPackageIdsis forwarded tomanageRequirement.This test covers the payload shape change but doesn’t verify the new field is actually propagated to the service call.
Suggested assertion
const res = await POST(req as never) expect(res.status).toBe(201) const json = (await res.json()) as { id: number } expect(json.id).toBe(42) + expect(mockManageRequirement).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + operation: 'create', + requirement: expect.objectContaining({ + requirementPackageIds: [1, 2], + }), + }), + )As per coding guidelines, "Add or update focused tests for changed DB/admin logic".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/requirements-list-route.test.ts` around lines 144 - 165, The test doesn't assert that the new field requirementPackageIds is passed to the service; update the 'creates requirement and returns 201' test to verify mockManageRequirement was called with an argument containing requirementPackageIds (e.g., use expect(mockManageRequirement).toHaveBeenCalledWith(expect.objectContaining({ requirementPackageIds: [1, 2] })) or similar partial matching), locating the call via the existing mockManageRequirement and the POST handler import in this test so the payload shape change is covered.tests/unit/edit-requirement-client.test.tsx (1)
31-47: ⚡ Quick winCover
initialRequirementPackageIdsin the mocked form.
makeVersion()now carriesversionRequirementPackages, but the test still doesn’t observe the new prop onRequirementForm. Please expose/assertinitialRequirementPackageIdsin the stub so this rename is actually covered.🧪 Suggested test update
default: (props: { baseRevisionToken?: string | null baseVersionId?: number | null mode: string requirementId?: number | string initialData?: Record<string, string | boolean> + initialRequirementPackageIds?: number[] }) => ( <div data-base-revision-token={props.baseRevisionToken ?? ''} data-base-version-id={props.baseVersionId ?? ''} data-initial-data={JSON.stringify(props.initialData)} + data-initial-requirement-package-ids={JSON.stringify( + props.initialRequirementPackageIds, + )} data-mode={props.mode} data-testid="req-form" /> ), }))Also applies to: 66-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/edit-requirement-client.test.tsx` around lines 31 - 47, The mocked RequirementForm doesn’t expose the new initialRequirementPackageIds prop from makeVersion; update the mock (the default export stub for RequirementForm) to accept props.initialRequirementPackageIds and render it as a data attribute (e.g., data-initial-requirement-package-ids={JSON.stringify(props.initialRequirementPackageIds ?? [])}) so tests can assert it; apply the same change to the other mock instance covering lines 66-95 and update assertions to check that data-initial-requirement-package-ids contains the expected package id array.tests/unit/requirements-id-route.test.ts (1)
111-138: ⚡ Quick winAssert the renamed payload field.
The fixture now sends
requirementPackageIds, but the expectation still ignores it. Please include the field inrequirementso this test fails if the route drops or renames the mapping.💡 Suggested assertion
requirement: expect.objectContaining({ baseRevisionToken: '11111111-1111-4111-8111-111111111111', baseVersionId: 10, + requirementPackageIds: [1, 2], }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/requirements-id-route.test.ts` around lines 111 - 138, Update the test assertion to include the renamed payload field so it fails if the route drops or renames it: when calling PUT (use the existing makeParams('1') and req fixture) assert that mockManageRequirement was called with an objectContaining requirement that includes requirementPackageIds: [1, 2] (alongside the already-checked baseRevisionToken and baseVersionId) so the expectation verifies the route preserves the requirementPackageIds mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/`[locale]/requirements/[id]/requirement-detail-client.tsx:
- Around line 411-422: The chips array generation (detailRequirementPackages)
can produce blank labels because
localName(versionRequirementPackage.requirementPackage) may be null; update the
mapping so the label falls back to the package id (e.g., label: localName(...)
?? String(versionRequirementPackage.requirementPackage.id)) and ensure
markerValue uses the same robust fallback logic, referencing selectedVersion,
versionRequirementPackage and buildDetailSectionContext as needed.
In `@app/api/requirement-packages/route.ts`:
- Around line 25-29: The route is casting untrusted request.json() to the
createRequirementPackage parameter type and calling createRequirementPackage(db,
body) directly; add runtime validation for the POST payload before the DAL call
(e.g., use a zod/schema validator or explicit checks) to verify required fields
like nameSv, nameEn and that ownerId is the correct type/format; if validation
fails, return a controlled 400 using NextResponse.json({ error: "...", details:
... }, { status: 400 }) and only call createRequirementPackage(db, body) when
validation passes so DB errors aren’t exposed for malformed input.
In `@app/api/requirements/route.ts`:
- Around line 58-61: The parsing of requirementPackageIds (the const
requirementPackageIds =
url.searchParams.getAll('requirementPackageIds').map(Number).filter(...))
accepts 0, negatives and non-integer numerics; update both the GET parsing and
the POST handler that reads body.requirementPackageIds to only accept positive
integers: parse each value with Number (or parseInt), ensure
Number.isInteger(value) && value > 0, and filter out anything else (or return a
400 if you prefer strict validation). Reference the requirementPackageIds
variable and the url.searchParams.getAll(...) usage in GET and the corresponding
POST location where body.requirementPackageIds is consumed and apply the same
validation logic.
In `@components/reports/pdf/PdfReportRenderer.tsx`:
- Around line 494-503: The PDF renderer currently maps requirement package names
with version.requirementPackages.map(s => (locale === 'sv' ? s.nameSv :
s.nameEn)), which can produce blank values when a translation is missing; update
the mapping in PdfReportRenderer (the block that builds the Text for requirement
packages) to use the same localized-name fallback logic used by the print
renderer (e.g., prefer the package's localized fallback field or fall back to
the other language, and filter out falsy/empty names before join) so that it
never renders empty entries and matches the print output.
In `@docs/database-schema.md`:
- Around line 1056-1074: The join table's foreign key behavior must be made
consistent by changing the requirement_package FK on
requirement_version_requirement_packages to ON DELETE CASCADE: update the
TypeORM entity in
lib/typeorm/entities/requirement-version-requirement-package.ts (the
relation/JoinColumn/ForeignKey definition around line 45) to include onDelete:
'CASCADE' for the package-side FK, modify migration 0003 to alter or recreate
the constraint so the requirement_package_id FK uses ON DELETE CASCADE, and
update docs/database-schema.md (the table description around line ~1397) to
document the FK as ON DELETE CASCADE.
In `@lib/reports/templates/history-template.ts`:
- Around line 69-74: The current mapping for requirementPackages uses
nameSv/nameEn defaulting to '' which produces blank entries; update the
transformation of version.versionRequirementPackages (used to set
requirementPackages) to skip items where both vs.requirementPackage.nameSv and
nameEn are missing and to prefer a non-empty locale as a fallback (e.g., nameSv
?? nameEn for nameSv and nameEn ?? nameSv for nameEn) so you only emit entries
with at least one real label and avoid creating objects with only empty strings.
In `@lib/reports/templates/review-template.ts`:
- Around line 152-165: The current diff compares requirement packages by their
localized names (oldRequirementPackages/newRequirementPackages) which causes
spurious diffs; instead, compute a stable comparison key from the related
package IDs by mapping reviewVersion.versionRequirementPackages and
baseVersion.versionRequirementPackages to requirementPackage.id (or another
stable identifier), sort and join those ID lists and use that result for the
equality check, while still building oldValue/newValue using getName(...) for
display; update the if-check to compare the ID-based strings but leave the
changes.push oldValue/newValue assignments unchanged so users see localized
names.
In `@lib/ui-terminology.ts`:
- Around line 268-273: The English labels under requirementPackage have
incorrect pluralization; update the en object for requirementPackage by changing
definitePlural and plural from "Requirements packages" to "Requirement packages"
and change singular from "Requirements package" to "Requirement package" (update
the keys definitePlural, plural, singular within the requirementPackage entry).
In `@messages/en.json`:
- Around line 752-756: The singular label for the terminology object
requirementPackage is incorrectly pluralized; update the
requirementPackage.singular value from "Requirements package" to the correctly
singular form "Requirement package" (and verify plural/definitePlural remain
"Requirements packages") so all uses of requirementPackage.singular render a
consistent singular label.
- Line 1261: Update the i18n string value for the key
"noRequirementPackagesAvailable" in messages/en.json to use the singular
modifier form; replace "No requirements packages available" with "No requirement
package available" so it reads consistently with the rest of the rename.
In `@typeorm/seed.mjs`:
- Around line 11797-11805: The seed row still uses the old term "Normal
driftscenario" — replace that string in the new package seed (the values array
containing 'Normal driftscenario') with the updated user-facing label (e.g.,
'Normal drift') and ensure any other occurrences in the same seed block or
nearby package seed entries are updated to remove "scenario" so the UI shows the
renamed term consistently.
- Around line 11656-11664: Update the seed data entry so the English singular
term is correct: change the value for singular_en from "Requirements package" to
"Requirement package" in the record that contains the fields including
'requirementPackage' and the associated Swedish and English plural forms,
ensuring the plural_en remains "Requirements packages" to keep plural forms
aligned.
---
Outside diff comments:
In `@app/`[locale]/requirement-packages/requirement-packages-client.tsx:
- Around line 162-193: The fetchLinkedRequirements callback currently saves
previousLinkedRequirements and restores it when a fetch fails, which can show
stale rows; change the error paths in fetchLinkedRequirements so that on non-ok
responses and in the catch block you do not call
setLinkedRequirements(previousLinkedRequirements) but instead clear the list
(e.g., setLinkedRequirements([])) and setLinkedRequirementsError(tc('error')) —
keep the requestId checks using linkedReqRequestId.current so updates only apply
to the latest request and still clear/set linkedRequirementsLoading as before.
In `@app/api/requirements/`[id]/route.ts:
- Around line 76-89: The mapping for requirementPackageIds in
app/api/requirements/[id]/route.ts should validate, dedupe and reject invalid
IDs before calling the service: convert each value to Number, filter out
NaN/non-positive (e.g., n => Number.isFinite(n) && n > 0), remove duplicates
(e.g., via Set) and, if the resulting array is empty, set requirementPackageIds
to undefined; apply the same stricter normalization pattern to normReferenceIds
as needed so only unique, positive integers flow to the service.
In `@components/RequirementsTable.tsx`:
- Around line 2934-2982: The requirement-package pill UI is rendered and
interactive even when filtering isn't supported (onFilterChange/updateFilter is
absent), producing inert controls; update the render guard around
requirementPackages (the whole block that maps requirementPackages and the clear
button) to only render when updateFilter (or the prop/function that wraps
onFilterChange) is present/allowed, or alternatively render non-interactive
disabled pills when updateFilter is undefined; reference the requirementPackages
map, the buttons that call updateFilter, and the (fv.requirementPackageIds ??
[]) clear button so you hide or disable those controls when filtering support is
not available.
In `@docs/admin-center.md`:
- Around line 121-129: The reference-data route list in the admin-center
documentation has omitted the "norm references" entry; update the list that
currently includes "areas", "types", "requirement packages", "statuses",
"quality characteristics", "business objects", and "implementation types" to
also include "norm references" so the summary matches the actual reference-data
routes (ensure the phrase "norm references" appears alongside the other bullet
items in the same list).
In `@typeorm/seed.mjs`:
- Around line 11954-11986: The catch/finally currently swallows transaction
cleanup failures: ensure rollback failures clear startedTransaction and are
propagated, and ensure commitTransaction failures in finally are not ignored. In
the catch block (around runner.rollbackTransaction) set startedTransaction =
false even if rollback throws and capture/rethrow the rollback error (or attach
it to the original error) instead of silently ignoring it; in the finally block,
when calling runner.commitTransaction(), do not swallow errors—catch to add
context (including currentTable/currentRowIndex/currentRow) and rethrow so
callers see that commit failed; still attempt runner.release() in a separate
try/catch that can ignore release errors. Use the existing symbols runner,
startedTransaction, commitTransaction, rollbackTransaction, currentTable,
currentRowIndex, and currentRow to locate and change the logic.
---
Nitpick comments:
In `@tests/integration/requirements-table-column-picker.spec.ts`:
- Around line 152-176: Replace the brittle exact-text selector that defines
requirementPackageFilter (currently using page.getByRole('button', { name:
'Mobil användning' })) with a stable data-* selector on the requirement-package
filter pill (e.g., use a dedicated attribute like data-requirement-package or
data-test-requirement-package) so the test queries by that attribute instead of
seeded text; update any assertions that reference requirementPackageFilter to
use the new selector and ensure the app fixtures add the corresponding data
attribute to the requirement-package pill component.
In `@tests/unit/edit-requirement-client.test.tsx`:
- Around line 31-47: The mocked RequirementForm doesn’t expose the new
initialRequirementPackageIds prop from makeVersion; update the mock (the default
export stub for RequirementForm) to accept props.initialRequirementPackageIds
and render it as a data attribute (e.g.,
data-initial-requirement-package-ids={JSON.stringify(props.initialRequirementPackageIds
?? [])}) so tests can assert it; apply the same change to the other mock
instance covering lines 66-95 and update assertions to check that
data-initial-requirement-package-ids contains the expected package id array.
In `@tests/unit/requirements-id-route.test.ts`:
- Around line 111-138: Update the test assertion to include the renamed payload
field so it fails if the route drops or renames it: when calling PUT (use the
existing makeParams('1') and req fixture) assert that mockManageRequirement was
called with an objectContaining requirement that includes requirementPackageIds:
[1, 2] (alongside the already-checked baseRevisionToken and baseVersionId) so
the expectation verifies the route preserves the requirementPackageIds mapping.
In `@tests/unit/requirements-list-route.test.ts`:
- Around line 144-165: The test doesn't assert that the new field
requirementPackageIds is passed to the service; update the 'creates requirement
and returns 201' test to verify mockManageRequirement was called with an
argument containing requirementPackageIds (e.g., use
expect(mockManageRequirement).toHaveBeenCalledWith(expect.objectContaining({
requirementPackageIds: [1, 2] })) or similar partial matching), locating the
call via the existing mockManageRequirement and the POST handler import in this
test so the payload shape change is covered.
In `@tests/unit/version-detail-client.test.tsx`:
- Around line 23-52: The test fixture returned by makeVersion currently sets
versionRequirementPackages: [] which leaves the new code path untested; update
makeVersion to include a single realistic package object in
versionRequirementPackages (e.g., with id, name, and any fields used by the
component under test) so the tests exercise the populated-package branch; ensure
the object shape matches RequirementVersionDetail's expected package structure
and leave overrides spread at the end to allow per-test customization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84879b94-9329-44ae-b9c5-a710c869e899
⛔ Files ignored due to path filters (2)
public/infographic-english.pngis excluded by!**/*.pngpublic/infographic-swedish.pngis excluded by!**/*.png
📒 Files selected for processing (95)
.github/skills/codebase-to-infograhipcs-prompt/SKILL.md.github/skills/codebase-to-infograhipcs-prompt/agents/openai.yaml.github/skills/run-spec-audit/references/integration-contracts.mdREADME.mdapp/[locale]/admin/admin-client.tsxapp/[locale]/requirement-packages/page.tsxapp/[locale]/requirement-packages/requirement-packages-client.tsxapp/[locale]/requirements/[id]/edit/edit-requirement-client.tsxapp/[locale]/requirements/[id]/requirement-detail-client.tsxapp/[locale]/requirements/requirements-client.tsxapp/[locale]/specifications/[slug]/requirements-specification-detail-client.tsxapp/[locale]/usage-scenarios/page.tsxapp/api/requirement-packages/[id]/route.tsapp/api/requirement-packages/route.tsapp/api/requirements/[id]/route.tsapp/api/requirements/route.tsapp/api/specifications/[id]/local-requirements/[localRequirementId]/route.tsapp/api/specifications/[id]/local-requirements/route.tsapp/api/specifications/[id]/report-items/route.tsapp/api/usage-scenarios/route.tscomponents/AiRequirementGenerator.tsxcomponents/RequirementDetailSections.tsxcomponents/RequirementForm.tsxcomponents/RequirementFormFields.tsxcomponents/RequirementsTable.tsxcomponents/SpecificationLocalRequirementDetailClient.tsxcomponents/SpecificationLocalRequirementForm.tsxcomponents/reports/pdf/PdfReportRenderer.tsxcomponents/reports/print/PrintReportRenderer.tsxcspell.jsoncdocs/admin-center.mddocs/arkitekturbeskrivning-kravhantering.mddocs/database-schema.mddocs/developer-mode-overlay.mddocs/dogfood-seed.mddocs/guide/README.mddocs/mcp-server-contributor-guide.mddocs/mcp-server-user-guide.mddocs/openshift-devspaces.mddocs/reference-data-and-ai.mddocs/requirements-ui-behaviour.mddocs/version-lifecycle-dates.mdlib/ai/requirement-prompt.tslib/ai/taxonomy.tslib/dal/requirement-packages.tslib/dal/requirements-specifications.tslib/dal/requirements.tslib/mcp/server.tslib/reports/data/fetch-deviation.tslib/reports/data/fetch-requirement.tslib/reports/templates/deviation-review-template.tslib/reports/templates/history-template.tslib/reports/templates/review-template.tslib/reports/templates/suggestion-history-template.tslib/reports/types.tslib/requirements/list-view.tslib/requirements/service.tslib/requirements/types.tslib/typeorm/entities/index.tslib/typeorm/entities/requirement-package.tslib/typeorm/entities/requirement-version-requirement-package.tslib/typeorm/entities/specification-local-requirement-requirement-package.tslib/typeorm/entities/usage-scenario.tslib/ui-terminology.tsmessages/en.jsonmessages/sv.jsontests/guide/generate-guide.spec.tstests/integration/requirements-table-column-picker.spec.tstests/quality/QUALITY.mdtests/quality/functional.test.tstests/unit/admin-client.test.tsxtests/unit/dogfood-seed.test.tstests/unit/edit-requirement-client.test.tsxtests/unit/mcp-http.test.tstests/unit/reference-data-developer-mode.test.tsxtests/unit/requirement-detail-client.test.tsxtests/unit/requirement-packages-client.test.tsxtests/unit/requirement-packages-dal.test.tstests/unit/requirement-prompt.test.tstests/unit/requirements-client.test.tsxtests/unit/requirements-dal.test.tstests/unit/requirements-id-route.test.tstests/unit/requirements-list-route.test.tstests/unit/requirements-service.test.tstests/unit/requirements-specification-detail-client.test.tsxtests/unit/requirements-specifications-dal.test.tstests/unit/specification-local-requirement-detail-client.test.tsxtests/unit/taxonomy-routes.test.tstests/unit/usage-scenarios-dal.test.tstests/unit/version-detail-client.test.tsxtypeorm/migrations/0001_initial_sqlserver.mjstypeorm/migrations/0003_explicit_fk_actions.mjstypeorm/seed-dogfood-build.mjstypeorm/seed-dogfood.mjstypeorm/seed.mjs
💤 Files with no reviewable changes (4)
- app/[locale]/usage-scenarios/page.tsx
- tests/unit/usage-scenarios-dal.test.ts
- lib/typeorm/entities/usage-scenario.ts
- app/api/usage-scenarios/route.ts
…irementPackageIds to usageScenarioIds in integration contracts
- Updated PdfReportRenderer to utilize localized names for requirement packages. - Enhanced admin center documentation to include norm references. - Modified database schema documentation for clarity on normal operations terminology. - Adjusted terminology in UI and messages for consistency in singular/plural forms of requirement packages. - Improved report templates to handle blank requirement packages and ensure fallback to available locales. - Updated tests to reflect changes in requirement package handling and ensure proper filtering in UI components. - Added validation for requirement package creation to handle invalid payloads. - Applied cascading delete actions in foreign key constraints for requirement packages in migrations.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
typeorm/seed.mjs (1)
381-405:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeed the renamed default column config too.
This block still has no
requirementPackageentry, so fresh databases will miss the default order/visibility metadata for the renamed column. The missingid = 12gap also suggests the old config was dropped instead of renamed in place.As per coding guidelines,
Seed requirement_list_column_defaults in typeorm/seed.mjs with the new column id, sort_order, and is_default_visibleandPreserve current seed-data meaning and identifiers wherever possible; document unavoidable drift explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typeorm/seed.mjs` around lines 381 - 405, The requirement_list_column_defaults seed is missing an entry for the renamed column "requirementPackage" (creating a gap at id = 12), so add a row in the rows array for requirement_list_column_defaults with id 12, column_id 'requirementPackage' and the correct sort_order and is_default_visible values (matching the intended default order/visibility for that column) and the same timestamp format as other rows; preserve the existing ids for other rows and use the same updated_at timestamp to minimize drift, and include this change in the seed.mjs requirement_list_column_defaults block so fresh databases receive the renamed column's default metadata.docs/database-schema.md (1)
231-238:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
requirement_packagesER entity is missingcreated_atandupdated_at.The table documentation at lines 616–617 documents both timestamp columns, but the ER diagram entity omits them. Every other entity with timestamps (e.g.,
owners,requirement_areas,requirements_specifications) includes them in the diagram.📝 Proposed fix
requirement_packages { integer id PK text name_sv text name_en text description_sv text description_en integer owner_id FK + text created_at + text updated_at }As per coding guidelines: "When any database schema, migration, or seed change is made, update the Entity-Relationship Diagram in the Mermaid
erDiagramindocs/database-schema.md: add/remove/rename entities, columns, and relationship lines."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/database-schema.md` around lines 231 - 238, The ER diagram entity for requirement_packages is missing the timestamp columns; update the mermaid erDiagram block for requirement_packages to include created_at and updated_at (matching the documented lines that show those columns) so it aligns with other entities like owners and requirement_areas; ensure the attributes are named exactly created_at and updated_at and placed in the requirement_packages entity within docs/database-schema.md.
🧹 Nitpick comments (5)
app/[locale]/requirement-packages/requirement-packages-client.tsx (1)
573-574: 💤 Low valueConsider adding title attribute for truncated descriptions.
The description cell uses
truncateclass but unlike the linked requirements table (line 501-505), it doesn't provide atitleattribute with the full text on hover. This creates an inconsistency in UX.♻️ Proposed fix for consistency
- <td className="py-3 px-4 text-secondary-600 dark:text-secondary-400 max-w-xs truncate"> - {getDescription(requirementPackage) || '—'} + <td + className="py-3 px-4 text-secondary-600 dark:text-secondary-400 max-w-xs truncate" + title={getDescription(requirementPackage) ?? undefined} + > + {getDescription(requirementPackage) ?? '—'} </td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`[locale]/requirement-packages/requirement-packages-client.tsx around lines 573 - 574, The description cell rendering uses the truncate class but lacks a hover tooltip; update the element that displays {getDescription(requirementPackage) || '—'} to include a title attribute with the full description (e.g., title={getDescription(requirementPackage) || '—'}) so hovering shows the complete text; ensure you add the title on the same element with className="... truncate" (the td or inner span) so behavior matches the linked requirements table.tests/unit/requirements-table.test.tsx (1)
3132-3167: ⚡ Quick winNew requirement package filter pill test — well-scoped, consider extending with active-state coverage.
The gate logic is correctly modelled: Context snippet 2 defines
hasFilters = !!onFilterChange; Context snippet 1 renders pills only whenrequirementPackages.length > 0 && hasFilters. The button name query,data-requirement-packageattribute check, andnot.toBeInTheDocument()absence assertion are all accurate.One optional gap: the test doesn't exercise the
aria-pressedtoggle (active vs. inactive state) whenfilterValues.requirementPackageIdsincludes or excludes the package id, nor theonFilterChangecallback invocation on click. Adding a follow-up assertion would give the pill's interactive behavior end-to-end coverage.🔧 Optional extension: active-state and callback coverage
rerender( <RequirementsTable getName={opt => opt.nameSv} locale="sv" onFilterChange={vi.fn()} requirementPackages={requirementPackages} rows={[makeRow()]} />, ) const requirementPackageFilter = screen.getByRole('button', { name: 'Mobil användning', }) expect(requirementPackageFilter).toHaveAttribute( 'data-requirement-package', '1', ) + + // Inactive by default (no filter values supplied) + expect(requirementPackageFilter).toHaveAttribute('aria-pressed', 'false') + + // Active when the package id is in filterValues + const onFilterChange = vi.fn() + rerender( + <RequirementsTable + filterValues={{ requirementPackageIds: [1] }} + getName={opt => opt.nameSv} + locale="sv" + onFilterChange={onFilterChange} + requirementPackages={requirementPackages} + rows={[makeRow()]} + />, + ) + expect(screen.getByRole('button', { name: 'Mobil användning' })).toHaveAttribute( + 'aria-pressed', + 'true', + ) + + // Click deactivates and calls onFilterChange + fireEvent.click(screen.getByRole('button', { name: 'Mobil användning' })) + expect(onFilterChange).toHaveBeenCalledWith( + expect.objectContaining({ requirementPackageIds: undefined }), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/requirements-table.test.tsx` around lines 3132 - 3167, The test currently verifies presence/absence of the requirement package pill but not its interactive behavior; extend the existing test for RequirementsTable to also assert the pill's active state and callback: render with onFilterChange and a filterValues object that includes the package id (filterValues.requirementPackageIds) to assert the button's aria-pressed=true, then simulate a user click on the button (use the same getByRole('button', { name: 'Mobil användning' })) and assert that onFilterChange was called with the updated filterValues (toggling the package id) and that aria-pressed toggles accordingly when you rerender with the new filterValues.tests/unit/report-templates.test.ts (1)
49-127: ⚡ Quick winConsider adding version-summary coverage for
buildReviewReport.Test 2 correctly validates that
buildReviewReportsuppresses ametadata-changessection when packages are the same by ID. However there's no test coveringbuildReviewReport'sversion-summarysection (emitted when no base version exists), specifically the locale fallback and blank-package filtering behavior. This mirrors the gap flagged inreview-template.ts'stoVersionSummary— adding a test here would drive the fix.➕ Suggested additional test case
it('falls back to available locale in review version-summary when no base version', () => { const model = buildReviewReport( makeRequirement([ makeVersion({ id: 1, status: 2, statusNameEn: 'Review', statusNameSv: 'Granskning', versionNumber: 1, versionRequirementPackages: [ { requirementPackage: { id: 1, nameEn: 'Mobile use', nameSv: null, }, }, { requirementPackage: { id: 2, nameEn: '', nameSv: '', }, }, ], }), ]), 'sv', ) const versionSummary = model.sections.find( section => section.type === 'version-summary', ) expect(versionSummary).toBeDefined() expect( versionSummary?.type === 'version-summary' ? versionSummary.version.requirementPackages : [], ).toEqual([{ nameEn: 'Mobile use', nameSv: 'Mobile use' }]) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/report-templates.test.ts` around lines 49 - 127, Add a unit test for buildReviewReport that mirrors the existing version-summary test for buildHistoryReport: create a review with a single version (no base version) whose versionRequirementPackages include one with only nameEn and another blank package, call buildReviewReport with locale 'sv', then assert the produced sections include a 'version-summary' whose version.requirementPackages filters out the blank package and falls back to nameEn for nameSv (i.e. [{ nameEn: 'Mobile use', nameSv: 'Mobile use' }]). This will exercise review-template.ts -> toVersionSummary behavior and ensure locale fallback + blank-package filtering are applied for review reports.app/api/requirements/route.ts (1)
214-216: ⚡ Quick winPOST can forward
requirementPackageIds: []to the service when all supplied values are invalid.
normalizePositiveIntegerIdsalways returns anumber[]. SoArray.isArray(body.requirementPackageIds) ? normalizePositiveIntegerIds(body.requirementPackageIds) : undefinedpasses[]tomanageRequirementwhen every value in the array is filtered out (e.g.,[0, -1, 'abc']). The GET path correctly guards with.length > 0 ? ... : undefined(lines 117–118), and the PUT handler in[id]/route.tsuses theundefined-returning variant of the helper, making this the sole inconsistency. If the service differentiates[]("remove all") fromundefined("no-op"), callers submitting an all-invalid array in a create request would silently associate zero packages rather than using any default.🛠️ Proposed fix
- requirementPackageIds: Array.isArray(body.requirementPackageIds) - ? normalizePositiveIntegerIds(body.requirementPackageIds) - : undefined, + requirementPackageIds: (() => { + if (!Array.isArray(body.requirementPackageIds)) return undefined + const ids = normalizePositiveIntegerIds(body.requirementPackageIds) + return ids.length > 0 ? ids : undefined + })(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/requirements/route.ts` around lines 214 - 216, In the POST handler in app/api/requirements/route.ts the requirementPackageIds field currently uses Array.isArray(body.requirementPackageIds) ? normalizePositiveIntegerIds(...) : undefined which can pass an empty [] when all inputs are filtered out; change this to mirror the GET/PUT behavior by checking the normalized array length and only passing the result to manageRequirement when length > 0 (i.e., Array.isArray(...) ? (normalizePositiveIntegerIds(...).length > 0 ? normalizePositiveIntegerIds(...) : undefined) ), referencing requirementPackageIds, normalizePositiveIntegerIds, and manageRequirement so the service receives undefined for no-valid-inputs instead of an empty array.app/api/requirements/[id]/route.ts (1)
15-32: ⚡ Quick win
normalizePositiveIntegerIdsis duplicated with a different contract from the one inroute.ts— extract to a shared utility.
route.tshas(values: Iterable<unknown>): number[](always returns an array, caller checks.length), while this file's version takesunknown, guards forArray.isArray, and returnsnumber[] | undefinedfor non-array or empty input. The core loop is identical. Having two diverging copies risks silent drift.Extract the canonical form to a shared module (e.g.,
lib/requirements/parse-ids.ts) and adjust each call-site to the appropriate wrapper:♻️ Suggested extraction
// lib/requirements/parse-ids.ts (new file) +export function normalizePositiveIntegerIds( + values: Iterable<unknown>, +): number[] { + const ids: number[] = [] + const seen = new Set<number>() + for (const value of values) { + const parsed = + typeof value === 'number' || typeof value === 'string' + ? Number(value) + : Number.NaN + if (Number.isInteger(parsed) && parsed > 0 && !seen.has(parsed)) { + seen.add(parsed) + ids.push(parsed) + } + } + return ids +} + +/** Convenience wrapper that returns `undefined` when the input is not an + * array or produces no valid IDs after normalization. */ +export function normalizePositiveIntegerIdsOrUndefined( + value: unknown, +): number[] | undefined { + if (!Array.isArray(value)) return undefined + const ids = normalizePositiveIntegerIds(value) + return ids.length > 0 ? ids : undefined +}Then in
[id]/route.ts:-function normalizePositiveIntegerIds(value: unknown): number[] | undefined { … } +import { normalizePositiveIntegerIdsOrUndefined as normalizePositiveIntegerIds } from '@/lib/requirements/parse-ids'And in
route.ts:-function normalizePositiveIntegerIds(values: Iterable<unknown>): number[] { … } +import { normalizePositiveIntegerIds } from '@/lib/requirements/parse-ids'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/requirements/`[id]/route.ts around lines 15 - 32, The duplicate implementations of normalizePositiveIntegerIds must be consolidated: extract the shared canonical parsing logic into a single exported function (e.g., parsePositiveIntegerIds) in a shared module (suggest lib/requirements/parse-ids.ts) and have both callers import it; then provide small wrappers at each call-site to match their contracts (one wrapper that accepts Iterable<unknown> and always returns number[] by iterating and returning [] when none, and another wrapper that accepts unknown, guards Array.isArray, calls the canonical parser and returns number[] | undefined for non-array or empty results). Ensure the exported function name (parsePositiveIntegerIds or normalizePositiveIntegerIds) is referenced consistently and only the thin adapters handle the differing input types/return shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/reports/templates/review-template.ts`:
- Around line 78-83: The requirementPackages mapping in toVersionSummary
currently uses simple null coalescing and a truthiness filter which lets
packages with one locale show blank in the other locale and may trigger
TypeScript strict errors; change the filter on vs.requirementPackage to a proper
type guard (e.g., the pattern used at line 162: (x): x is T => Boolean(...)) so
TS knows requirementPackage is present, then map each package using cross-locale
fallbacks (set nameSv = requirementPackage.nameSv ?? requirementPackage.nameEn
?? '' and nameEn = requirementPackage.nameEn ?? requirementPackage.nameSv ?? '')
and exclude any package where both resolved names are empty so fully-blank
packages are filtered out.
In `@lib/ui-terminology.ts`:
- Around line 85-87: The nav message-binding for requirementPackage is using the
wrong snake_case key; update the 'requirementPackage' entry in
lib/ui-terminology.ts (the requirementPackage object with plural/singular
bindings) to use ['nav', 'requirementPackages'] instead of ['nav',
'requirement_packages'] so it matches the i18n message file's camelCase key and
restores terminology customization for requirement packages.
In `@typeorm/seed.mjs`:
- Around line 11808-11811: The seed failure currently throws an error that
serializes the entire seed row (including emails and large text) into logs;
update the error construction in seed.mjs so it does NOT include the full row
object but instead logs only the table name, the seed row index (or other loop
position) and any primary key values for that row; locate the place that
performs the insert and throws (the seed insertion loop / error throw in
seed.mjs) and replace the detailed serialization with a concise message
containing table name, row index, and PKs (avoid JSON.stringify(row) or
similar).
---
Outside diff comments:
In `@docs/database-schema.md`:
- Around line 231-238: The ER diagram entity for requirement_packages is missing
the timestamp columns; update the mermaid erDiagram block for
requirement_packages to include created_at and updated_at (matching the
documented lines that show those columns) so it aligns with other entities like
owners and requirement_areas; ensure the attributes are named exactly created_at
and updated_at and placed in the requirement_packages entity within
docs/database-schema.md.
In `@typeorm/seed.mjs`:
- Around line 381-405: The requirement_list_column_defaults seed is missing an
entry for the renamed column "requirementPackage" (creating a gap at id = 12),
so add a row in the rows array for requirement_list_column_defaults with id 12,
column_id 'requirementPackage' and the correct sort_order and is_default_visible
values (matching the intended default order/visibility for that column) and the
same timestamp format as other rows; preserve the existing ids for other rows
and use the same updated_at timestamp to minimize drift, and include this change
in the seed.mjs requirement_list_column_defaults block so fresh databases
receive the renamed column's default metadata.
---
Nitpick comments:
In `@app/`[locale]/requirement-packages/requirement-packages-client.tsx:
- Around line 573-574: The description cell rendering uses the truncate class
but lacks a hover tooltip; update the element that displays
{getDescription(requirementPackage) || '—'} to include a title attribute with
the full description (e.g., title={getDescription(requirementPackage) || '—'})
so hovering shows the complete text; ensure you add the title on the same
element with className="... truncate" (the td or inner span) so behavior matches
the linked requirements table.
In `@app/api/requirements/`[id]/route.ts:
- Around line 15-32: The duplicate implementations of
normalizePositiveIntegerIds must be consolidated: extract the shared canonical
parsing logic into a single exported function (e.g., parsePositiveIntegerIds) in
a shared module (suggest lib/requirements/parse-ids.ts) and have both callers
import it; then provide small wrappers at each call-site to match their
contracts (one wrapper that accepts Iterable<unknown> and always returns
number[] by iterating and returning [] when none, and another wrapper that
accepts unknown, guards Array.isArray, calls the canonical parser and returns
number[] | undefined for non-array or empty results). Ensure the exported
function name (parsePositiveIntegerIds or normalizePositiveIntegerIds) is
referenced consistently and only the thin adapters handle the differing input
types/return shapes.
In `@app/api/requirements/route.ts`:
- Around line 214-216: In the POST handler in app/api/requirements/route.ts the
requirementPackageIds field currently uses
Array.isArray(body.requirementPackageIds) ? normalizePositiveIntegerIds(...) :
undefined which can pass an empty [] when all inputs are filtered out; change
this to mirror the GET/PUT behavior by checking the normalized array length and
only passing the result to manageRequirement when length > 0 (i.e.,
Array.isArray(...) ? (normalizePositiveIntegerIds(...).length > 0 ?
normalizePositiveIntegerIds(...) : undefined) ), referencing
requirementPackageIds, normalizePositiveIntegerIds, and manageRequirement so the
service receives undefined for no-valid-inputs instead of an empty array.
In `@tests/unit/report-templates.test.ts`:
- Around line 49-127: Add a unit test for buildReviewReport that mirrors the
existing version-summary test for buildHistoryReport: create a review with a
single version (no base version) whose versionRequirementPackages include one
with only nameEn and another blank package, call buildReviewReport with locale
'sv', then assert the produced sections include a 'version-summary' whose
version.requirementPackages filters out the blank package and falls back to
nameEn for nameSv (i.e. [{ nameEn: 'Mobile use', nameSv: 'Mobile use' }]). This
will exercise review-template.ts -> toVersionSummary behavior and ensure locale
fallback + blank-package filtering are applied for review reports.
In `@tests/unit/requirements-table.test.tsx`:
- Around line 3132-3167: The test currently verifies presence/absence of the
requirement package pill but not its interactive behavior; extend the existing
test for RequirementsTable to also assert the pill's active state and callback:
render with onFilterChange and a filterValues object that includes the package
id (filterValues.requirementPackageIds) to assert the button's
aria-pressed=true, then simulate a user click on the button (use the same
getByRole('button', { name: 'Mobil användning' })) and assert that
onFilterChange was called with the updated filterValues (toggling the package
id) and that aria-pressed toggles accordingly when you rerender with the new
filterValues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3755e16-d532-440c-a263-1790db070224
📒 Files selected for processing (26)
app/[locale]/requirement-packages/requirement-packages-client.tsxapp/[locale]/requirements/[id]/requirement-detail-client.tsxapp/api/requirement-packages/route.tsapp/api/requirements/[id]/route.tsapp/api/requirements/route.tscomponents/RequirementsTable.tsxcomponents/reports/pdf/PdfReportRenderer.tsxdocs/admin-center.mddocs/database-schema.mddocs/guide/README.mdlib/reports/templates/history-template.tslib/reports/templates/review-template.tslib/typeorm/entities/requirement-version-requirement-package.tslib/ui-terminology.tsmessages/en.jsonmessages/sv.jsontests/integration/requirements-table-column-picker.spec.tstests/unit/edit-requirement-client.test.tsxtests/unit/report-templates.test.tstests/unit/requirement-prompt.test.tstests/unit/requirements-id-route.test.tstests/unit/requirements-list-route.test.tstests/unit/requirements-table.test.tsxtests/unit/taxonomy-routes.test.tstypeorm/migrations/0003_explicit_fk_actions.mjstypeorm/seed.mjs
✅ Files skipped from review due to trivial changes (4)
- docs/guide/README.md
- app/[locale]/requirements/[id]/requirement-detail-client.tsx
- tests/integration/requirements-table-column-picker.spec.ts
- messages/en.json
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/admin-center.md
- components/reports/pdf/PdfReportRenderer.tsx
- lib/reports/templates/history-template.ts
- tests/unit/requirements-id-route.test.ts
- tests/unit/edit-requirement-client.test.tsx
- tests/unit/requirement-prompt.test.ts
- messages/sv.json
- components/RequirementsTable.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
app/api/requirements/route.ts (1)
18-20: 💤 Low value
normalizePositiveIntegerIdsis a redundant pass-through wrapper.The function body is a single unconditional delegate to
parsePositiveIntegerIds, adding no guard, rename semantics, or transformation. Callers can referenceparsePositiveIntegerIdsdirectly, which is already imported on line 10. ThenormalizeOptionalPositiveIntegerIdswrapper (lines 22-29) is justified because it adds real logic (array guard + undefined coercion), but this one isn't.♻️ Proposed refactor
-function normalizePositiveIntegerIds(values: Iterable<unknown>): number[] { - return parsePositiveIntegerIds(values) -} - function normalizeOptionalPositiveIntegerIds(Then update the one call-site:
- const requirementPackageIds = normalizePositiveIntegerIds( + const requirementPackageIds = parsePositiveIntegerIds( url.searchParams.getAll('requirementPackageIds'), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/requirements/route.ts` around lines 18 - 20, Remove the redundant wrapper function normalizePositiveIntegerIds and update its call-site(s) to call parsePositiveIntegerIds directly; locate the function normalizePositiveIntegerIds and any references to it, delete the function declaration, and replace usages with parsePositiveIntegerIds (no additional guards needed since parsePositiveIntegerIds is already imported/available).tests/unit/seed-database.test.ts (3)
12-17: ⚡ Quick winPrefer Vitest's
rejectsAPI over manual try/catchThe try/catch pattern initialises
message = '', meaning the two negative assertions on Lines 22–23 silently pass ifseedDatabasenever throws. Usingrejectsmakes the "must throw" contract explicit and produces a clearer failure message.♻️ Proposed refactor
- let message = '' - try { - await seedDatabase(executor) - } catch (error) { - message = error instanceof Error ? error.message : String(error) - } - - expect(message).toContain( - "Seed failed while seeding table='norm_references' rowIndex=0 pk={id=1}: insert failed", - ) - expect(message).not.toContain('row=') - expect(message).not.toContain('SFS 2018:218') + await expect(seedDatabase(executor)).rejects.toThrow( + "Seed failed while seeding table='norm_references' rowIndex=0 pk={id=1}: insert failed", + ) + await expect(seedDatabase(executor)).rejects.not.toThrow('row=') + await expect(seedDatabase(executor)).rejects.not.toThrow('SFS 2018:218')Note: if calling
seedDatabasethree times is undesirable (e.g., side-effects or slowness), capture the rejection first:const err = await seedDatabase(executor).catch((e: unknown) => e instanceof Error ? e : new Error(String(e)) ) expect(err.message).toContain("Seed failed ...") expect(err.message).not.toContain('row=') expect(err.message).not.toContain('SFS 2018:218')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/seed-database.test.ts` around lines 12 - 17, Replace the manual try/catch that sets message = '' with Vitest's rejects API (or capture the rejection once) so the test explicitly asserts that seedDatabase(executor) rejects; e.g., use await expect(seedDatabase(executor)).rejects.toThrow() and then check the error message with expects, or if you must inspect the message without invoking seedDatabase multiple times, capture the rejection via const err = await seedDatabase(executor).catch(e => e instanceof Error ? e : new Error(String(e))) and then assert on err.message; update assertions that reference message to use the rejection result and remove the initial message = '' pattern.
19-21: 💤 Low valueHardcoded table/rowIndex/pk couples the test to seed-data ordering
table='norm_references' rowIndex=0 pk={id=1}will break silently if a new seed table is inserted beforenorm_references, or if thenorm_referencesseed array is reordered. Consider asserting only on the stable parts of the message (e.g., the error suffix: insert failed) and adding a separate, clearly named test for theseedPositionDetailformatting if positional accuracy matters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/seed-database.test.ts` around lines 19 - 21, The test currently asserts the full seed-position string (expect(message).toContain("Seed failed while seeding table='norm_references' rowIndex=0 pk={id=1}: insert failed")), which couples it to seed ordering; change this assertion to only check the stable error suffix (e.g., expect(message).toContain(': insert failed') or similar) and, if positional formatting must be validated, add a separate focused unit test for the seedPositionDetail formatter (a new test named like "seedPositionDetail formats table/rowIndex/pk correctly") that asserts the exact formatting of seedPositionDetail output, leaving the main failure-message test resilient to seed ordering changes.
6-10: ⚡ Quick winRewrite executor mock using idiomatic Vitest pattern
The test currently uses try/catch with an initialized
messagevariable (Lines 12–17). Use Vitest'srejects.toThrow()pattern instead for clarity:Suggested pattern
await expect(seedDatabase(executor)).rejects.toThrow( "Seed failed while seeding table='norm_references' rowIndex=0 pk={id=1}: insert failed" ) expect(message).not.toContain('row=') expect(message).not.toContain('SFS 2018:218')Note: If using
rejects.toThrow(), extract the error message within the assertion for the negative checks, or structure as separatetoThrow()+ additional assertions on the caught error.The executor mock (Lines 6–10) is correctly minimal—it intentionally provides only
queryto exercise the query-failure path without triggering thecreateQueryRunnerbranch. No TypeScript type issue arises; the mock satisfies the duck-typed contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/seed-database.test.ts` around lines 6 - 10, Replace the current try/catch style in the test with Vitest's promise rejection assertions: use await expect(seedDatabase(executor)).rejects.toThrow("Seed failed while seeding table='norm_references' rowIndex=0 pk={id=1}: insert failed") to assert the thrown error, and then capture the thrown error for the negative checks (e.g., const err = await seedDatabase(executor).catch(e => e); expect(err.message).not.toContain('row='); expect(err.message).not.toContain('SFS 2018:218')). Keep the existing minimal executor mock and reference the seedDatabase and executor symbols when updating the test.tests/unit/ui-terminology.test.ts (2)
63-63: 💤 Low valueStale/inconsistent label in test input data.
Line 63:
requirementPackages: 'Requirements packages'uses the plural'Requirements'(likely a copy-paste artefact from an older label). While the test still passes becausetoMatchObjectonly checks the overridden output, keeping mismatched base-message strings in test fixtures makes the intent of the overlay harder to read.✏️ Suggested fix
- requirementPackages: 'Requirements packages', + requirementPackages: 'Requirement packages',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/ui-terminology.test.ts` at line 63, Update the test fixture's stale label for the requirementPackages key in tests/unit/ui-terminology.test.ts: replace the pluralized string "Requirements packages" with the corrected base-message string (e.g., "Requirement packages") so the fixture matches the current overlay/base messages and makes the test intent clearer.
37-102: ⚡ Quick win
terminology.requirementPackageoutput not asserted.The new
requirementPackageentry is added tonormalizeUiTerminologyinput but neither thesvMessagesnorenMessagestoMatchObjectexpectations verify that the resultingterminology.requirementPackageobject is present and correctly shaped. The existing entries (status,description) all have theirterminology.*sub-objects verified. Omitting this check means the test would pass even ifapplyUiTerminologyMessagessilently drops the new key from theterminologyoutput.🧪 Suggested addition to the `enMessages` expectation
expect(enMessages).toMatchObject({ nav: { requirementPackages: 'Delivery bundles', statuses: 'Lifecycle states', }, requirement: { description: 'Requirement text', status: 'Lifecycle state', }, terminology: { description: { definitePlural: 'Requirement texts', plural: 'Requirement texts', singular: 'Requirement text', }, + requirementPackage: { + definitePlural: 'Delivery bundles', + plural: 'Delivery bundles', + singular: 'Delivery bundle', + }, }, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/ui-terminology.test.ts` around lines 37 - 102, The test adds a new terminology entry requirementPackage to the normalizeUiTerminology input but never asserts it in the outputs, so update the enMessages and svMessages expectations to include a terminology.requirementPackage object check; modify the toMatchObject calls for enMessages and svMessages (where applyUiTerminologyMessages is invoked) to assert terminology.requirementPackage has the correct singular/plural/definitePlural values for both 'en' and 'sv', mirroring how terminology.status and terminology.description are already asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/reports/templates/review-template.ts`:
- Around line 173-196: The current oldRequirementPackages/newRequirementPackages
use getName(requirementPackage, locale) which can return empty for packages
missing that locale; change their mapping to use the same locale fallback that
toVersionSummary uses (e.g. derive the display name via
toVersionSummary(vs.requirementPackage, locale)?.name ?? '' or otherwise call
getName with the fallback) so removed/added package diffs show a non-empty
oldValue/newValue; update the map expressions in the versionRequirementPackages
handling (oldRequirementPackages and newRequirementPackages) to use that
fallback approach.
In `@tests/unit/requirements-list-route.test.ts`:
- Around line 193-200: The current assertion using expect.objectContaining({
requirementPackageIds: undefined }) won't fail if the key is missing; instead
retrieve the actual call args from mockManageRequirement
(mockManageRequirement.mock.calls) and assert the requirement object truly lacks
the key, e.g. extract the second arg's requirement and use
expect(...).not.toHaveProperty('requirementPackageIds') (or assert property is
undefined via a direct property access on the captured requirement) to ensure
the key is absent; update the test around mockManageRequirement to perform this
stricter check.
In `@typeorm/seed.mjs`:
- Around line 11657-11663: Add a new seed entry for the UI column id
"requirementPackage" into the requirement_list_column_defaults seed data so
environments get default visibility and sort configuration; update the seed data
structure that populates requirement_list_column_defaults to include an
object/row with column_id "requirementPackage" and set appropriate sort_order
(e.g., numeric position) and is_default_visible (true/false per UX), ensuring it
follows the same shape and ordering as the other seeded entries so
migrations/seeds run without schema mismatch.
---
Nitpick comments:
In `@app/api/requirements/route.ts`:
- Around line 18-20: Remove the redundant wrapper function
normalizePositiveIntegerIds and update its call-site(s) to call
parsePositiveIntegerIds directly; locate the function
normalizePositiveIntegerIds and any references to it, delete the function
declaration, and replace usages with parsePositiveIntegerIds (no additional
guards needed since parsePositiveIntegerIds is already imported/available).
In `@tests/unit/seed-database.test.ts`:
- Around line 12-17: Replace the manual try/catch that sets message = '' with
Vitest's rejects API (or capture the rejection once) so the test explicitly
asserts that seedDatabase(executor) rejects; e.g., use await
expect(seedDatabase(executor)).rejects.toThrow() and then check the error
message with expects, or if you must inspect the message without invoking
seedDatabase multiple times, capture the rejection via const err = await
seedDatabase(executor).catch(e => e instanceof Error ? e : new Error(String(e)))
and then assert on err.message; update assertions that reference message to use
the rejection result and remove the initial message = '' pattern.
- Around line 19-21: The test currently asserts the full seed-position string
(expect(message).toContain("Seed failed while seeding table='norm_references'
rowIndex=0 pk={id=1}: insert failed")), which couples it to seed ordering;
change this assertion to only check the stable error suffix (e.g.,
expect(message).toContain(': insert failed') or similar) and, if positional
formatting must be validated, add a separate focused unit test for the
seedPositionDetail formatter (a new test named like "seedPositionDetail formats
table/rowIndex/pk correctly") that asserts the exact formatting of
seedPositionDetail output, leaving the main failure-message test resilient to
seed ordering changes.
- Around line 6-10: Replace the current try/catch style in the test with
Vitest's promise rejection assertions: use await
expect(seedDatabase(executor)).rejects.toThrow("Seed failed while seeding
table='norm_references' rowIndex=0 pk={id=1}: insert failed") to assert the
thrown error, and then capture the thrown error for the negative checks (e.g.,
const err = await seedDatabase(executor).catch(e => e);
expect(err.message).not.toContain('row=');
expect(err.message).not.toContain('SFS 2018:218')). Keep the existing minimal
executor mock and reference the seedDatabase and executor symbols when updating
the test.
In `@tests/unit/ui-terminology.test.ts`:
- Line 63: Update the test fixture's stale label for the requirementPackages key
in tests/unit/ui-terminology.test.ts: replace the pluralized string
"Requirements packages" with the corrected base-message string (e.g.,
"Requirement packages") so the fixture matches the current overlay/base messages
and makes the test intent clearer.
- Around line 37-102: The test adds a new terminology entry requirementPackage
to the normalizeUiTerminology input but never asserts it in the outputs, so
update the enMessages and svMessages expectations to include a
terminology.requirementPackage object check; modify the toMatchObject calls for
enMessages and svMessages (where applyUiTerminologyMessages is invoked) to
assert terminology.requirementPackage has the correct
singular/plural/definitePlural values for both 'en' and 'sv', mirroring how
terminology.status and terminology.description are already asserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: beb17e02-fec8-4b93-b253-8988b892d4bf
📒 Files selected for processing (14)
app/[locale]/requirement-packages/requirement-packages-client.tsxapp/api/requirements/[id]/route.tsapp/api/requirements/route.tsdocs/database-schema.mdlib/reports/templates/review-template.tslib/requirements/parse-ids.tslib/ui-terminology.tstests/unit/report-templates.test.tstests/unit/requirements-list-route.test.tstests/unit/requirements-parse-ids.test.tstests/unit/requirements-table.test.tsxtests/unit/seed-database.test.tstests/unit/ui-terminology.test.tstypeorm/seed.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- app/api/requirements/[id]/route.ts
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
typeorm/seed.mjs (1)
11657-11663:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeed the missing default config for the new
requirementPackagecolumn.Line 11657 introduces the new UI column id, but
requirement_list_column_defaultsstill has no matching row at all — the existing gaps atid = 12andsort_order = 10make the omission pretty clear. Seeded environments will miss the default sort/visibility config for this field.As per coding guidelines,
typeorm/seed.mjs:Seed requirement_list_column_defaults in typeorm/seed.mjs with the new column id, sort_order, and is_default_visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typeorm/seed.mjs` around lines 11657 - 11663, Add a new seed row into the requirement_list_column_defaults data in typeorm/seed.mjs for the new UI column id 'requirementPackage' to fill the missing default configuration; create an entry with id = 12 (matching the existing gap), set sort_order = 10, and set is_default_visible to the intended default (true/false per spec), and include any other required fields (e.g., label or locale keys) to match the shape of other entries so seeded environments receive the correct sort/visibility for requirementPackage.
🧹 Nitpick comments (2)
lib/reports/templates/review-template.ts (1)
176-205: 💤 Low valueOptional: DRY the duplicated old/new package computations.
The
flatMapblocks at lines 176–183 and 186–193 are identical except for the source version, and the ID computations at 196–200 and 201–205 mirror each other. Extracting two small helpers (e.g.,collectPackageNames(version, locale)andcollectPackageIds(version)) keepscomputeMetadataChangessymmetric and reduces the surface for future drift between old/new sides.♻️ Proposed refactor
+ const collectPackageNames = ( + version: RequirementReportData['versions'][number], + ): string => + version.versionRequirementPackages + .flatMap(vs => { + const name = getRequirementPackageDisplayName( + vs.requirementPackage, + locale, + ) + return name ? [name] : [] + }) + .sort() + .join(', ') + const collectPackageIds = ( + version: RequirementReportData['versions'][number], + ): string => + version.versionRequirementPackages + .map(vs => vs.requirementPackage?.id) + .filter((id): id is number => Number.isInteger(id)) + .sort((a, b) => a - b) + .join(',') + - const oldRequirementPackages = baseVersion.versionRequirementPackages - .flatMap(vs => { - const name = getRequirementPackageDisplayName( - vs.requirementPackage, - locale, - ) - return name ? [name] : [] - }) - .sort() - .join(', ') - const newRequirementPackages = reviewVersion.versionRequirementPackages - .flatMap(vs => { - const name = getRequirementPackageDisplayName( - vs.requirementPackage, - locale, - ) - return name ? [name] : [] - }) - .sort() - .join(', ') - const oldRequirementPackageIds = baseVersion.versionRequirementPackages - .map(vs => vs.requirementPackage?.id) - .filter((id): id is number => Number.isInteger(id)) - .sort((a, b) => a - b) - .join(',') - const newRequirementPackageIds = reviewVersion.versionRequirementPackages - .map(vs => vs.requirementPackage?.id) - .filter((id): id is number => Number.isInteger(id)) - .sort((a, b) => a - b) - .join(',') + const oldRequirementPackages = collectPackageNames(baseVersion) + const newRequirementPackages = collectPackageNames(reviewVersion) + const oldRequirementPackageIds = collectPackageIds(baseVersion) + const newRequirementPackageIds = collectPackageIds(reviewVersion)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/reports/templates/review-template.ts` around lines 176 - 205, The four duplicated blocks building oldRequirementPackages/newRequirementPackages and oldRequirementPackageIds/newRequirementPackageIds should be replaced by two small helper functions: implement collectPackageNames(version, locale) that performs the flatMap -> getRequirementPackageDisplayName -> filter -> sort -> join logic, and collectPackageIds(version) that maps to requirementPackage?.id -> filters Number.isInteger -> sorts numerically -> joins; then call these helpers from computeMetadataChanges to produce oldRequirementPackages/newRequirementPackages and oldRequirementPackageIds/newRequirementPackageIds, keeping existing variable names (oldRequirementPackages, newRequirementPackages, oldRequirementPackageIds, newRequirementPackageIds) and reusing getRequirementPackageDisplayName and versionRequirementPackages.tests/unit/report-templates.test.ts (1)
125-165: 💤 Low valueOptional: also assert that no other metadata-change rows were emitted to harden the stable-ID test.
The current assertion only checks that no
metadata-changessection exists, which is correct given the defaults. If someone later wires a non-null default intomakeVersion(e.g.,category), this test would silently pass for the wrong reason because a metadata-change row could appear from the unrelated field while the package row is still suppressed. Consider asserting on a more targeted property, e.g., that the metadata-changes section (if present) does not include aKravpaket/Requirements packagesrow, so the stable-ID intent stays explicit.♻️ Proposed adjustment
- expect( - model.sections.some(section => section.type === 'metadata-changes'), - ).toBe(false) + const metadataChanges = model.sections.find( + section => section.type === 'metadata-changes', + ) + const changes = + metadataChanges?.type === 'metadata-changes' ? metadataChanges.changes : [] + expect( + changes.some(c => c.field === 'Requirements packages' || c.field === 'Kravpaket'), + ).toBe(false)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/report-templates.test.ts` around lines 125 - 165, Update the test that uses buildReviewReport/makeRequirement/makeVersion to also assert that if a 'metadata-changes' section exists it does not contain a row about requirement packages: locate model.sections (type === 'metadata-changes') and either assert that no such section contains an entry with the label used for requirement packages (e.g., "Kravpaket" or "Requirements packages") or assert that the metadata-changes section is absent OR its rows filter out any row whose key or label matches the requirement-package field; reference the existing test case that compares stable IDs so the new assertion specifically verifies the absence of a requirement-package change row rather than relying on the whole-section absence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@typeorm/seed.mjs`:
- Around line 11657-11663: Add a new seed row into the
requirement_list_column_defaults data in typeorm/seed.mjs for the new UI column
id 'requirementPackage' to fill the missing default configuration; create an
entry with id = 12 (matching the existing gap), set sort_order = 10, and set
is_default_visible to the intended default (true/false per spec), and include
any other required fields (e.g., label or locale keys) to match the shape of
other entries so seeded environments receive the correct sort/visibility for
requirementPackage.
---
Nitpick comments:
In `@lib/reports/templates/review-template.ts`:
- Around line 176-205: The four duplicated blocks building
oldRequirementPackages/newRequirementPackages and
oldRequirementPackageIds/newRequirementPackageIds should be replaced by two
small helper functions: implement collectPackageNames(version, locale) that
performs the flatMap -> getRequirementPackageDisplayName -> filter -> sort ->
join logic, and collectPackageIds(version) that maps to requirementPackage?.id
-> filters Number.isInteger -> sorts numerically -> joins; then call these
helpers from computeMetadataChanges to produce
oldRequirementPackages/newRequirementPackages and
oldRequirementPackageIds/newRequirementPackageIds, keeping existing variable
names (oldRequirementPackages, newRequirementPackages, oldRequirementPackageIds,
newRequirementPackageIds) and reusing getRequirementPackageDisplayName and
versionRequirementPackages.
In `@tests/unit/report-templates.test.ts`:
- Around line 125-165: Update the test that uses
buildReviewReport/makeRequirement/makeVersion to also assert that if a
'metadata-changes' section exists it does not contain a row about requirement
packages: locate model.sections (type === 'metadata-changes') and either assert
that no such section contains an entry with the label used for requirement
packages (e.g., "Kravpaket" or "Requirements packages") or assert that the
metadata-changes section is absent OR its rows filter out any row whose key or
label matches the requirement-package field; reference the existing test case
that compares stable IDs so the new assertion specifically verifies the absence
of a requirement-package change row rather than relying on the whole-section
absence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00c6d6be-2a59-49dd-a3ec-a6249f9d4bf8
📒 Files selected for processing (7)
app/api/requirements/route.tslib/reports/templates/review-template.tstests/unit/report-templates.test.tstests/unit/requirements-list-route.test.tstests/unit/seed-database.test.tstests/unit/ui-terminology.test.tstypeorm/seed.mjs
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is