feat: rename to requirements specification - #153
Merged
Conversation
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:
WalkthroughRenames and retargets the "package" domain to "specification" across the codebase: DB schema, TypeORM entities, DAL, API routes, services, hooks, clients, UI, reports, i18n, seeds, migrations, and tests; all package-scoped endpoints/fields are replaced by specification-scoped equivalents. ChangesPackages → Specifications Refactor
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Client
participant NextAPI as Next.js API
participant Service as Requirements Service
participant DAL as DAL
participant DB as SQL Server
Browser->>NextAPI: POST /api/specifications/{slug}/items (add requirements)
NextAPI->>Service: addToSpecification(input)
Service->>DAL: linkRequirementsToSpecificationAtomically(specId, requirementIds)
DAL->>DB: INSERT requirements_specification_items + (maybe) specification_needs_references (transaction)
DB-->>DAL: OK / inserted ids
DAL-->>Service: { addedCount, skippedCount, skippedIds }
Service-->>NextAPI: { addedCount, skippedCount, message }
NextAPI-->>Browser: 200 OK { summary }
sequenceDiagram
autonumber
participant Browser as Client
participant NextAPI as Next.js API
participant DAL as DAL
participant DB as SQL Server
Browser->>NextAPI: GET /api/specification-item-deviations/{itemId}
NextAPI->>DAL: listDeviationsForSpecificationItem(itemId)
DAL->>DB: SELECT union deviations + specification_local_requirement_deviations
DB-->>DAL: rows
DAL-->>NextAPI: normalized rows (isSpecificationLocal, itemRef, spec metadata)
NextAPI-->>Browser: 200 OK [deviations]
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
|
![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: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (18)
docs/dogfood-seed.md (1)
90-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish the terminology rename in the KH/KH-POC prose.
This section still uses “package” wording in sentences describing the two
Kravunderlag. Replace those remaining mentions with “specification” terms so
the section is consistent with the updated table/entity names.Suggested wording adjustments
- The main package containing all 59 Krav from the dogfood inventory. + The main specification containing all 59 Krav from the dogfood inventory. - sit on top of this package to demonstrate PoC-specific divergence + sit on top of this specification to demonstrate PoC-specific divergence🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dogfood-seed.md` around lines 90 - 104, The prose under the "Kravunderlag" section still uses the word "package" when describing `KH` and `KH-POC`; update those sentences to use "specification" (or "specification" phrasing) instead to match the renamed entities, e.g., change "The main package containing..." to "The main specification containing..." and "A smaller curated subset (17 Krav) that demonstrates the *Införande* lifecycle" to reference `KH-POC` as a specification; keep the references to `specification_local_requirements` as-is and ensure all occurrences of "package" in this block (including the two sentences describing `KH` and `KH-POC`) are replaced with consistent "specification" terminology.app/api/specifications/route.ts (1)
1-33:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAPI route path conflicts with locale-captured segment naming.
This route uses
/api/specifications, whileapp/[locale]/specifications/...exists.
Per project routing safety rules, this can be captured by[locale]and return HTML
404 instead of the API handler in affected setups. Move this API namespace to a
conflict-free second segment and update callers.As per coding guidelines, "Do not place API routes at paths that can be matched by
app/[locale]/...... Use conflict-free top-level nouns if there is overlap."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/specifications/route.ts` around lines 1 - 33, This API route conflicts with app/[locale]/specifications because the top-level segment "specifications" can be captured by [locale]; move the endpoint to a conflict-free second segment (e.g., change the public path from /api/specifications to /api/v1/specifications or /api/internal/specifications), update all callers to the new path, and keep the existing handler functions GET and POST (and their usage of createPackage, isSlugTaken, listPackages, getRequestSqlServerDataSource) unchanged so only the route namespace changes.docs/reports.md (1)
56-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse one domain term consistently (“specification”) in this report doc.
These updated sections still mix package/specification wording (detail view,
context refs, and filename template). Please normalize terminology so route/data
contract descriptions stay unambiguous.Example normalization
- Available from the print dropdown in the package detail view + Available from the print dropdown in the specification detail view - Includes both library requirements linked into the package and + Includes both library requirements linked into the specification and - package-context item references such as ... + specification-context item references such as ... - List (package): `{localized label} {package name} {specification ID}.pdf` + List (specification): `{localized label} {specification name} {specification ID}.pdf`Also applies to: 155-163, 187-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reports.md` around lines 56 - 66, The doc mixes "package" and "specification" terms; standardize to use "specification" everywhere: update phrases like "package detail view", "package metadata", "package name", "package-local requirements", "package detail", and the filename template to read "specification" (e.g., "specification detail view", "specification metadata", "specification-local requirements"); also update route and context descriptions such as "Print opens a dedicated route; PDF is generated inline in the package detail view" to reference the specification route/view consistently, and apply the same normalization to the other referenced sections that currently use mixed terminology.app/[locale]/specifications/specifications-client.tsx (1)
320-339:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle taxonomy fetch failures independently.
A single rejected request here aborts the entire
Promise.all(...), so one flaky taxonomy endpoint leaves all three dropdowns empty and produces an unhandled rejection from the effect. Using per-request handling orPromise.allSettled(...)would preserve partial data and let you surface a controlled error state.Suggested direction
- const [areasRes, typesRes, statusesRes] = await Promise.all([ + const [areasRes, typesRes, statusesRes] = await Promise.allSettled([ apiFetch('/api/specification-responsibility-areas'), apiFetch('/api/specification-implementation-types'), apiFetch('/api/specification-lifecycle-statuses'), ]) - if (areasRes.ok && isMountedRef.current) + if (areasRes.status === 'fulfilled' && areasRes.value.ok && isMountedRef.current) setResponsibilityAreas( - ((await areasRes.json()) as { areas?: TaxonomyItem[] }).areas ?? [], + ((await areasRes.value.json()) as { areas?: TaxonomyItem[] }).areas ?? [], ) - if (typesRes.ok && isMountedRef.current) + if (typesRes.status === 'fulfilled' && typesRes.value.ok && isMountedRef.current) setImplementationTypes( - ((await typesRes.json()) as { types?: TaxonomyItem[] }).types ?? [], + ((await typesRes.value.json()) as { types?: TaxonomyItem[] }).types ?? [], ) - if (statusesRes.ok && isMountedRef.current) + if (statusesRes.status === 'fulfilled' && statusesRes.value.ok && isMountedRef.current) setLifecycleStatuses( - ((await statusesRes.json()) as { statuses?: TaxonomyItem[] }) + ((await statusesRes.value.json()) as { statuses?: TaxonomyItem[] }) .statuses ?? [], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[locale]/specifications/specifications-client.tsx around lines 320 - 339, fetchTaxonomies uses Promise.all so one failing apiFetch aborts all three and causes unhandled rejections; change it to handle each request independently (e.g., use Promise.allSettled or wrap each apiFetch in try/catch) and for each result check ok and isMountedRef.current before calling setResponsibilityAreas, setImplementationTypes, or setLifecycleStatuses, parse JSON only when the response is ok, set an empty default when a single request fails, and surface/log per-endpoint errors so one flaky taxonomy endpoint doesn't clear all dropdowns.app/[locale]/specifications/[slug]/specification-edit-panel.tsx (1)
116-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the same submit re-entry guard used in the list client.
This handler does not bail out when
isSubmittingis alreadytrue, so a quick double submit can send two PUTs and fireonSavedtwice before the button disable propagates.Suggested fix
const handleSubmit = async (event: FormEvent<HTMLFormElement>) => { event.preventDefault() + if (isSubmitting) return setIsSubmitting(true) setSlugError(null) setSubmitError(null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[locale]/specifications/[slug]/specification-edit-panel.tsx around lines 116 - 120, The submit handler handleSubmit should bail out immediately if a submission is already in progress to prevent duplicate PUTs; add a re-entry guard at the top of handleSubmit that checks the isSubmitting state and returns early when true, keeping the existing event.preventDefault() behavior and state updates (setIsSubmitting, setSlugError, setSubmitError) intact; ensure the rest of the function still sets setIsSubmitting(true) only when proceeding and that setIsSubmitting(false) is called on error/after completion so the guard remains effective.typeorm/seed.mjs (1)
664-667:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep this needs reference consistent with the seeded specification.
requirements_specificationsrow 9 is a development initiative (APIGW-UTV-2026), but this text still describes a procurement. That makes the representative seed data internally contradictory.Suggested fix
- 'Rate limiting är ett primärt krav för API-gateway-upphandlingen', + 'Rate limiting är ett primärt krav för API-gateway-utvecklingen',As per coding guidelines, "Ensure every table in the schema has representative, idempotent seed rows" and "Preserve current seed-data meaning and identifiers wherever possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typeorm/seed.mjs` around lines 664 - 667, The seeded row in requirements_specifications that uses spec id 9 currently describes a procurement but must match the development initiative APIGW-UTV-2026; update the text value (the string currently "Rate limiting är ett primärt krav för API-gateway-upphandlingen") to a development-focused phrasing (e.g., mention implementation or development for APIGW-UTV-2026) while leaving the numeric identifiers (21, 9) and the timestamp '2026-04-17 20:07:00' unchanged so the seed remains idempotent and consistent with the spec.typeorm/seed-dogfood-build.mjs (1)
292-312:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
specification_local_requirementsrows are one column too wide.Line 309-Line 312 append four trailing timestamps after
note, but the specification-local schema only has three trailing timestamp columns (status_updated_at,created_at,updated_at). That leaves every seeded local requirement row with one extra cell, which can misalign column-based inserts.Suggested fix
locals.rows.push([ localId, pl.pkg, uniqueId, seq, k.area, k.desc + (pl.descSuffix || ''), k.ac + (pl.acSuffix || ''), k.cat, k.type, k.qc, k.risk, k.test ? 1 : 0, k.vm, needsRefId, pl.item, pl.note || null, SEED_TS, SEED_TS, SEED_TS, - SEED_TS, ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typeorm/seed-dogfood-build.mjs` around lines 292 - 312, The array pushed in locals.rows (the array starting with localId, pl.pkg, ... , pl.note || null, SEED_TS, SEED_TS, SEED_TS, SEED_TS) is one element too wide; remove the extra trailing SEED_TS so only three timestamps remain to match the specification_local_requirements columns (status_updated_at, created_at, updated_at). Locate the array push (the locals.rows.push call) and delete the fourth SEED_TS after pl.note || null so the inserted row has exactly three trailing timestamp values.app/api/specifications/[id]/report-items/route.ts (1)
1-192:⚠️ Potential issue | 🟠 MajorMove this endpoint family out of
app/api/specifications.The repo already has locale-routed pages under
app/[locale]/specifications/...(page.tsx, [slug]/page.tsx, and subpaths), creating a path conflict with this/api/specifications/...endpoint. App Router's dynamic[locale]segment can match "api" and cause requests to resolve through locale routing and return HTML/404 instead of this JSON handler. Move to a conflict-free top-level noun inapp/api/(e.g.,app/api/specification-reports/...).As per coding guidelines: "Do not place API routes at paths that can be matched by
app/[locale]/.... Turbopack's[locale]dynamic segment captures every path segment includingapi, returning a 404 HTML page instead of the route handler."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/specifications/`[id]/report-items/route.ts around lines 1 - 192, This API endpoint (route.ts with exported GET) lives under app/api/specifications/... which conflicts with app/[locale]/specifications dynamic routing; move the entire folder and its route.ts to a top-level, locale-safe path such as app/api/specification-reports/[id]/report-items/route.ts (or similar) so requests hit the JSON handler; after moving, update any callers/clients that request /api/specifications/... to the new path and ensure imports referencing functions like GET, parsePackageItemRef, getPackageById, getSpecificationLocalRequirementDetail, and mapSpecificationLocalRequirementToReportData remain correct (adjust relative paths if you moved helper modules) and run tests to validate the new route.typeorm/migrations/0001_initial_sqlserver.mjs (1)
4-152:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't rewrite already-applied migrations for this rename.
This changes
0001in place, so databases that have already recordedInitialSqlServerSchema1713720000000will never see these table/column/FK renames. They will keep the old package-named schema while the renamed entities and routes now targetrequirements_specifications,requirements_specification_items, etc., which makes upgrades break. Please restore the historical migrations and add a new forward migration that renames the existing objects withsp_rename/ALTER TABLEinstead. As per coding guidelines, "Migrations live intypeorm/migrations/(one.mjsfile per migration)." and "Prefer SQL ServerALTER TABLE … ADD,ALTER COLUMN, andsp_renameover drop-and-recreate for renames and additions in migrations to keep foreign key safety."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typeorm/migrations/0001_initial_sqlserver.mjs` around lines 4 - 152, The migration 0001 (InitialSqlServerSchema1713720000000) was modified in place to rename tables/columns (e.g., requirements_specifications, requirements_specification_items) which breaks upgrades for DBs that already applied the original migration; revert the historical 0001 migration back to its original state (restore original table/column/FK names in the existing 0001_initial_sqlserver.mjs) and create a new forward migration file that performs the renames using SQL Server-safe statements (sp_rename, ALTER TABLE ... ADD/ALTER COLUMN, and ALTER TABLE ... DROP/ADD CONSTRAINT where necessary) to rename each affected object and update FKs rather than dropping/recreating them so existing databases will see the changes during migration.lib/dal/specification-item-statuses.ts (1)
60-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe linked-status lookup is broken and incomplete.
requirements_specification_itemsuses
requirements_specification_id, notspecification_id, so the join in
getLinkedPackageItems()will fail. On top of that, both linked-usage
queries only inspectrequirements_specification_items, even though
specification_local_requirementsalso carries
specification_item_status_id, so linked counts/details are underreported
and a delete can clear local statuses without warning.Also applies to: 90-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/dal/specification-item-statuses.ts` around lines 60 - 69, The linked-status queries are wrong: change any join/filter that references specification_id to use requirements_specification_id and include both sources that carry specification_item_status_id (requirements_specification_items and specification_local_requirements) so counts and details include both tables; update countLinkedPackageItems and getLinkedPackageItems to aggregate (e.g., UNION ALL or two-source JOIN/UNION) across requirements_specification_items and specification_local_requirements using requirements_specification_id as the FK and group by specification_item_status_id (or statusId) to produce correct linked counts and avoid underreporting or missing deletes.app/api/specifications/[id]/local-requirements/[localRequirementId]/route.ts (1)
206-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap DELETE failures to JSON as well.
Line 206 calls the DAL without a catch block, so database/service failures bypass the error handling pattern you already use in
PUTand return a framework 500 instead of a stable JSON payload.Suggested fix
- const deleted = await deleteSpecificationLocalRequirement( - db, - specificationId, - numericLocalRequirementId, - ) - if (!deleted) { - return NextResponse.json({ error: 'Not found' }, { status: 404 }) - } - - return NextResponse.json({ ok: true }) + try { + const deleted = await deleteSpecificationLocalRequirement( + db, + specificationId, + numericLocalRequirementId, + ) + if (!deleted) { + return NextResponse.json({ error: 'Not found' }, { status: 404 }) + } + + return NextResponse.json({ ok: true }) + } catch (error) { + if (isRequirementsServiceError(error)) { + return NextResponse.json( + { error: error.message }, + { status: error.status }, + ) + } + + console.error('Failed to delete specification-local requirement', error) + return NextResponse.json( + { error: 'Failed to delete specification-local requirement' }, + { status: 500 }, + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/specifications/`[id]/local-requirements/[localRequirementId]/route.ts around lines 206 - 215, The delete path currently calls deleteSpecificationLocalRequirement(db, specificationId, numericLocalRequirementId) with no error handling so DB/service exceptions escape and produce a framework 500; wrap that call in a try/catch, return the same JSON-shaped error responses you use in the PUT handler (e.g., NextResponse.json({ error: '...' }, { status: 500 }) for server errors) and keep the existing 404 branch when deleted is falsy; reference deleteSpecificationLocalRequirement, db, specificationId, numericLocalRequirementId and NextResponse.json when making the change.app/[locale]/requirements/[id]/_detail/RequirementActionRail.tsx (1)
179-188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the dev marker to the renamed action.
The button now renders the specification action, but the marker still publishes
value: 'add to package', so Developer Mode/scanner consumers will keep seeing the old name.Suggested fix
{...devMarker({ context: detailContext, name: 'detail action', priority: 360, - value: 'add to package', + value: 'add to specification', })}As per coding guidelines, When changing visible UI elements, labels, roles, or layout surfaces, update curated devMarker(...) calls or scanner heuristics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[locale]/requirements/[id]/_detail/RequirementActionRail.tsx around lines 179 - 188, The devMarker call for the specification button still publishes the old value 'add to package'; locate the devMarker(...) invocation inside RequirementActionRail (the button that calls onOpenAddToSpecification and uses detailContext/name 'detail action') and update the value field to the renamed action (e.g., 'add to specification') so the scanner/dev-mode consumers match the visible UI change, leaving the rest of the devMarker properties intact.tests/unit/requirements-specification-detail-client.test.tsx (1)
368-375:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename the dev-marker expectation from
edit package.This suite now targets the specification detail view, but the asserted developer-mode value still uses the old term. That makes a partial rename look correct in tests.
Suggested expectation update
expect(editButton).toHaveAttribute( 'data-developer-mode-value', - 'edit package', + 'edit specification', )As per coding guidelines,
{tests/unit/**/*.{ts,tsx},tests/integration/**/*.spec.ts}: When changing visible UI elements, labels, roles, or layout surfaces, update the relevant unit and integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/requirements-specification-detail-client.test.tsx` around lines 368 - 375, Update the dev-marker expectation for the edit button: in tests/unit/requirements-specification-detail-client.test.tsx change the asserted data-developer-mode-value on editButton from 'edit package' to 'edit specification' so the test reflects the specification detail view; locate the assertion that calls expect(editButton).toHaveAttribute('data-developer-mode-value', ...) and replace the string accordingly.tests/unit/requirements-service.test.ts (1)
1078-1108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate these success-message assertions to
specification.The renamed service methods still pin the outward-facing text to
Package, so this test will either fail once the copy is corrected or keep an incomplete rename green.Suggested expectation update
expect(JSON.parse(result.message)).toMatchObject({ - lines: ['Added 1 requirement to package IAM-PACKAGE.'], - title: 'Requirements Added to Package', + lines: ['Added 1 requirement to specification IAM-PACKAGE.'], + title: 'Requirements Added to Specification', }) ... expect(JSON.parse(result.message)).toMatchObject({ - lines: ['Removed 1 requirement from package IAM-PACKAGE.'], - title: 'Requirements Removed from Package', + lines: ['Removed 1 requirement from specification IAM-PACKAGE.'], + title: 'Requirements Removed from Specification', })As per coding guidelines,
{tests/unit/requirements-service.test.ts,tests/unit/mcp-http.test.ts}: Updatetests/unit/requirements-service.test.tsandtests/unit/mcp-http.test.tsfor service or MCP output changes.Also applies to: 1111-1129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/requirements-service.test.ts` around lines 1078 - 1108, Update the test assertions in tests/unit/requirements-service.test.ts that check the outward-facing message from RequirementsService.addToSpecification: change occurrences of "Package" to "Specification" and the lines text from "Added X requirement to package <SPEC>." to "Added X requirement to specification <SPEC>." Also update the JSON title expectation from 'Requirements Added to Package' to 'Requirements Added to Specification' so the expected output from addToSpecification and any uses of linkRequirementsToPackageAtomically remain consistent with the renamed service wording.lib/mcp/server.ts (1)
1319-1354:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinish the MCP rename in the contract, not just in the tool IDs.
These tools are now exposed as
...specification..., but their descriptions, titles, several.describe(...)strings, and the list output schema still saypackage/packages. MCP clients read that metadata as the contract, so the current mix is misleading and leaves the outward-facing rename incomplete.Representative fixes
- 'List all requirements specifications, optionally filtered by name. Returns id, uniqueId (slug), names, item count, responsibility area, and implementation type for each package.', + 'List all requirements specifications, optionally filtered by name. Returns id, uniqueId (slug), names, item count, responsibility area, and implementation type for each specification.', - 'Case-insensitive substring filter applied to both Swedish and English package names.', + 'Case-insensitive substring filter applied to both Swedish and English specification names.', - packages: z.array( + specifications: z.array( - title: 'Get Package Items', + title: 'Get Specification Items', - '... Identify the package with specificationId ...', + '... Identify the specification with specificationId ...', - 'Numeric requirement IDs (not uniqueId strings) to add to the package.', + 'Numeric requirement IDs (not uniqueId strings) to add to the specification.', - title: 'Add Requirements to Package', + title: 'Add Requirements to Specification', - title: 'Remove Requirements from Package', + title: 'Remove Requirements from Specification',As per coding guidelines,
{lib/mcp/**/*.ts,lib/requirements/**/*.ts,lib/dal/**/*.ts,app/api/**/*.ts}: Keep MCP tooldescription,inputSchema,outputSchema, and field.describe(...)text aligned with the handler/service behavior.Also applies to: 1385-1443, 1477-1529, 1563-1604
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mcp/server.ts` around lines 1319 - 1354, The tool's public contract mixes "package/packages" with "specification/specifications": update the title, description string, all .describe(...) texts in the inputSchema, and the outputSchema structure and field names so they consistently use "requirement specification"/"requirements specification" (singular/plural) or the chosen "specification" term; specifically adjust the tool title "List Requirements Specifications", any description text, the inputSchema.nameSearch .describe(...) string, and the outputSchema top-level field currently named "packages" and its item property descriptions (e.g., businessNeedsReference, implementationType, responsibilityArea, uniqueId) so the schema keys/describe strings and returned message match the renamed concept across the handler and contract (ensure z.object keys and z.describe strings reflect the new term and keep types unchanged).components/RequirementsTable.tsx (1)
2360-2412:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the status label when no color is configured.
This branch only renders a value when
specificationItemStatusColoris present. Since the color is nullable, rows with a valid status name but no configured color will look unset and show—. Render the label whenever it exists, and only make the dot conditional.Suggested fix
- {statusColor ? ( + {statusLabel ? ( <span className="inline-flex items-center gap-1.5"> - <span - aria-hidden="true" - className="inline-block w-2.5 h-2.5 rounded-full shrink-0" - style={{ backgroundColor: statusColor }} - /> - {statusLabel ?? '—'} + {statusColor ? ( + <span + aria-hidden="true" + className="inline-block w-2.5 h-2.5 rounded-full shrink-0" + style={{ backgroundColor: statusColor }} + /> + ) : null} + {statusLabel} </span> ) : ( '—' )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/RequirementsTable.tsx` around lines 2360 - 2412, The display branch for the 'specificationItemStatus' column currently shows the status label only when specificationItemStatusColor is present, causing rows with a name but no color to render '—'; change the rendering so the statusLabel is always shown when non-null and only the colored dot (the inline-block with backgroundColor using statusColor) is conditional on specificationItemStatusColor, keeping the existing fallback '—' only when statusLabel is absent; locate this logic in the return block that renders the <td> (near SpecificationItemStatusSelect and variables statusId/statusLabel/statusColor) and adjust the JSX to render the label unconditionally while wrapping the dot element in a condition on statusColor.lib/requirements/service.ts (1)
300-334:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winService output still mixes “package/paket” with “specification/kravunderlag”.
These helpers and summaries are returned to clients for list/get/add/remove flows, so the rename is still incomplete in API/MCP-facing output (
Package Requirements,Added ... to package,... från/fran paket, etc.). Please switch the remaining human-text generators to specification terminology consistently.As per coding guidelines, "Update human-text generators in
lib/requirements/service.tsandlib/mcp/server.tswhen the property name or value appears in generated text."Also applies to: 1635-1642, 1708-1713, 1804-1810, 1863-1867
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/requirements/service.ts` around lines 300 - 334, Update the human-facing strings so they use "specification"/"kravunderlag" instead of "package"/"paket": modify getPackageWord to return 'specification'/'specifications' for non-sv and keep 'kravunderlag' for sv, and update getPackageServiceTitle (cases 'add','items','remove' and default) to use English phrases like "Specification Added", "Specification Items" (or "Specifications" as appropriate) and Swedish equivalents ("Krav tillagda i underlag" → change to use "kravunderlag"/appropriate Swedish wording instead of "underlag/paket"); apply the same string replacements in the corresponding generators in lib/mcp/server.ts and the other locations listed (around the referenced ranges) so all API/MCP-facing outputs consistently use specification/kravunderlag.app/[locale]/specifications/[slug]/requirements-specification-detail-client.tsx (1)
101-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVersion the persisted column preferences for the specification view.
This rename changes the saved column semantics, but the component still hydrates from the old
requirement-packages.*.v2keys andreadStoredCols()accepts any stored array as-is. Existing users can therefore carry stale package-view column ids into this page instead of cleanly falling back to the new defaults.Suggested fix
-const LEFT_VISIBLE_COLS_KEY = 'requirement-packages.visibleColumns.left.v2' -const RIGHT_VISIBLE_COLS_KEY = 'requirement-packages.visibleColumns.right.v2' +const LEFT_VISIBLE_COLS_KEY = 'requirements-specifications.visibleColumns.left.v3' +const RIGHT_VISIBLE_COLS_KEY = 'requirements-specifications.visibleColumns.right.v3'As per coding guidelines, "If UI view state is stored in browser storage and its semantics change materially, version the storage key instead of reusing incompatible stored data."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[locale]/specifications/[slug]/requirements-specification-detail-client.tsx around lines 101 - 130, The component currently reuses the old storage key semantics and accepts any stored array, so stale/incompatible column ids can be hydrated; update LEFT_VISIBLE_COLS_KEY and RIGHT_VISIBLE_COLS_KEY to a new version suffix (e.g., change '.v2' → '.v3') and harden readStoredCols: after JSON.parse, validate that parsed is an array and that every item is a known RequirementColumnId (compare against the allowed set derived from DEFAULT_LEFT_COLS, DEFAULT_RIGHT_COLS or an explicit enum/list); if validation fails (unknown ids, non-string entries, or empty array), return the provided fallback so the view falls back to new defaults. Ensure the window check and try/catch remain.
🧹 Nitpick comments (8)
docs/reference-data-and-ai.md (1)
80-110: ⚡ Quick winFix internal doc inconsistency: “Package” vs “Specification” taxonomy DAL naming
Section 3 now lists
specification-*DAL sources, but the validation variance table and surrounding text still referencepackage-*DAL filenames and “package workflow gates” (e.g., lines 101-104 and 107-110). This will confuse readers about which implementation actually contains the described behavior.Proposed doc update
-## 3 — Package Taxonomy Lookups +## 3 — Specification Taxonomy Lookups ... - `package-lifecycle-statuses`: returns row count (number). - - `package-implementation-types`: returns `void`. - - `package-responsibility-areas`: returns `void`. + - `specification-lifecycle-statuses`: returns row count (number). + - `specification-implementation-types`: returns `void`. + - `specification-responsibility-areas`: returns `void`. ... -This variance is intentional. Lifecycle statuses are -safety-critical (they determine package workflow gates), while -implementation types and responsibility areas are informational +This variance is intentional. Lifecycle statuses are +safety-critical (they determine specification workflow gates), while +implementation types and responsibility areas are informational taxonomy values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference-data-and-ai.md` around lines 80 - 110, The document uses `lib/dal/specification-implementation-types.ts`, `lib/dal/specification-lifecycle-statuses.ts`, and `lib/dal/specification-responsibility-areas.ts` in the Sources header but still refers to `package-*` filenames and "package workflow gates" in the validation table and surrounding text; update all occurrences of `package-lifecycle-statuses.ts`, `package-implementation-types.ts`, and `package-responsibility-areas.ts` to the corresponding `specification-...` filenames and change the phrase "package workflow gates" to "specification workflow gates" (or another consistent term) so the filenames and explanatory text consistently reference the specification DALs (ensure changes cover the table rows, captions, and the two lines describing safety-critical lifecycle statuses).cspell.jsonc (2)
37-48: ⚡ Quick winRemove redundant lowercase duplicates (caseSensitive=false).
Because
caseSensitiveisfalse, adding bothKravunderlagslistaandkravunderlagslista(and similarlyKravunderlagsdetaljandkravunderlagsdetalj) is likely redundant noise. Consider keeping only one casing variant for each lemma to reduce list churn/merge conflicts.🛠️ Proposed cleanup
"Kravunderlagslista", - "kravunderlagslista", "Kravunderlagsdetalj", - "kravunderlagsdetalj", "Kravunderlagsdetaljsidan",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cspell.jsonc` around lines 37 - 48, The wordlist contains redundant case variants because caseSensitive is false; remove the lowercase duplicates so only one casing variant remains for each lemma (e.g., keep "Kravunderlagslista" and remove "kravunderlagslista", keep "Kravunderlagsdetalj" and remove "kravunderlagsdetalj")—update the entries for "Kravunderlagslista"/"kravunderlagslista" and "Kravunderlagsdetalj"/"kravunderlagsdetalj" to a single canonical form to reduce noise and future merge conflicts.
429-435: ⚡ Quick winDouble-check spelling:
kravunderlagmedförfattaremay be missing “s”.In the new entries,
kravunderlagmedförfattare(line 432) differs from the earlierKravunderlagsmedförfattare(line 39) by missing the “s” inunderlags. If that’s unintentional, cspell may not learn the correct term form you expect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cspell.jsonc` around lines 429 - 435, The cspell entry "kravunderlagmedförfattare" is missing the "s" present in the earlier "Kravunderlagsmedförfattaren"; update the entry in the list to "kravunderlagsmedförfattare" to match the intended stem (or, if both forms are valid, add the correctly spelled variant "kravunderlagsmedförfattare" in addition to any existing capitalized form) so cspell consistently recognizes the term (look for the tokens "kravunderlagmedförfattare" and "Kravunderlagsmedförfattaren" to locate the items).lib/specification-item-status-constants.ts (1)
2-11: ⚡ Quick winAlign JSDoc wording with renamed “specification” domain.
The constants were renamed, but comments still describe “package-item status”.
Update the JSDoc text to avoid domain ambiguity.Suggested doc-only diff
- * Seed ID for the "Included" / "Inkluderad" package-item status. - * Every newly added package item starts here. + * Seed ID for the "Included" / "Inkluderad" specification-item status. + * Every newly added specification item starts here. ... - * Seed ID for the "Deviated" / "Avviken" package-item status. - * Only selectable when the package item has an approved deviation. + * Seed ID for the "Deviated" / "Avviken" specification-item status. + * Only selectable when the specification item has an approved deviation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/specification-item-status-constants.ts` around lines 2 - 11, Update the JSDoc comments for the renamed specification domain constants to remove "package-item" wording and refer to the specification item status domain instead; edit the comments above DEFAULT_SPECIFICATION_ITEM_STATUS_ID and DEVIATED_SPECIFICATION_ITEM_STATUS_ID so they describe "specification item status" (e.g., "Seed ID for the 'Included' specification-item status" and "Seed ID for the 'Deviated' specification-item status") and keep the existing explanatory lines about when each status is used.components/reports/pdf/PdfReportRenderer.tsx (1)
295-297: 🏗️ Heavy liftAvoid hardcoded label text in the PDF cover renderer.
Line 296 introduces visible copy inline (
'Kravunderlag-ID'/'Specification ID'). Please bind this through translation keys (or pass a prelocalized label via report model) to stay consistent with the localization contract.As per coding guidelines, "Bind all visible labels to translation keys. Do not hardcode labels in list headers, edit forms, inline detail panes, detail pages, CSV, service output, or MCP HTML".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/reports/pdf/PdfReportRenderer.tsx` around lines 295 - 297, Replace the hardcoded label string in the PdfReportRenderer Text element with a localized value: locate the Text with styles.fieldLabel in the PdfReportRenderer component and bind its content to a translation key (e.g. use the i18n/translation helper like t('reports.cover.specificationId')) or accept a prelocalized label from the report model prop and render that instead of the inline `'Kravunderlag-ID'` / `'Specification ID'`; ensure the new key exists in the locale files and remove the inline literal so all visible labels are served via the translation contract.tests/unit/requirements-specification-items-route.test.ts (1)
39-39: ⚡ Quick winAdd a GET case for the renamed specification route.
This suite now exercises
POST/DELETEonly, but the handler’sGETpath also changed to resolve a specification id and merge deviation counts. A rename/regression there would currently go unnoticed.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-specification-items-route.test.ts` at line 39, Add a focused unit test for the GET handler in the specifications items route: import the GET export from '@/app/api/specifications/[id]/items/route' (in addition to POST and DELETE) and add a test that calls GET with a request and params containing the renamed/expected specification id, then assert the response resolves the correct specification id and returns merged deviation counts (verify merged counts and any changed fields). Use the same setup/mocks as the existing POST/DELETE tests (mock DB/admin services) and assert status and JSON body to catch regressions in the GET path that resolves specification id and merges deviation counts.tests/unit/requirements-specification-item-route.test.ts (1)
29-29: ⚡ Quick winAdd PATCH coverage for the renamed item-status payload.
This file pins the renamed
GETresponse, butPATCHalso changed frompackageItemStatusIdtospecificationItemStatusIdand now resolves against a specification id. A focused PATCH test would lock that contract down.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-specification-item-route.test.ts` at line 29, The test currently only imports GET from '@/app/api/specifications/[id]/items/[itemId]/route' — add a focused unit test that imports and invokes the PATCH handler from the same route and asserts the new payload/contract: send a PATCH request body using specificationItemStatusId (not packageItemStatusId) and target the request against the specification id (the [id] param) so the handler resolves status against the specification; update assertions to verify the DB/admin call receives specificationItemStatusId and the correct specification id was used in the update/resolution.components/SpecificationLocalRequirementDetailClient.tsx (1)
96-99: ⚡ Quick winUse the shared default specification-item status constant here.
This gate duplicates the Included status id as
1. If the seeded/default id ever changes, edit/delete will silently flip behavior in the UI while the DAL and tests keep using the shared constant.Suggested fix
import { devMarker } from '@/lib/developer-mode-markers' import { apiFetch } from '@/lib/http/api-fetch' +import { DEFAULT_SPECIFICATION_ITEM_STATUS_ID } from '@/lib/specification-item-status-constants' ... - const INCLUDED_PACKAGE_ITEM_STATUS_ID = 1 const t = useTranslations('requirement') ... const canMutateLocalRequirement = - requirement.specificationItemStatusId === INCLUDED_PACKAGE_ITEM_STATUS_ID && + requirement.specificationItemStatusId === + DEFAULT_SPECIFICATION_ITEM_STATUS_ID && !hasPendingDeviationAlso applies to: 657-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/SpecificationLocalRequirementDetailClient.tsx` around lines 96 - 99, Replace the local hard-coded INCLUDED_PACKAGE_ITEM_STATUS_ID with the shared default "Included" specification-item status constant: remove the local const declaration and import the project-wide default Included status constant, then update all usages of INCLUDED_PACKAGE_ITEM_STATUS_ID (including the other occurrences mentioned) to reference the shared constant instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2016f540-b5c0-4ae2-bacf-f89ea44f53f2
📒 Files selected for processing (150)
app/[locale]/admin/admin-client.tsxapp/[locale]/package-item-statuses/page.tsxapp/[locale]/requirement-packages/page.tsxapp/[locale]/requirements/[id]/_detail/AddToSpecificationDialog.tsxapp/[locale]/requirements/[id]/_detail/RequirementActionRail.tsxapp/[locale]/requirements/[id]/_detail/RequirementReportMenu.tsxapp/[locale]/requirements/[id]/_detail/SpecificationDeviationRail.tsxapp/[locale]/requirements/[id]/_detail/types.tsapp/[locale]/requirements/[id]/_detail/use-add-to-specification-dialog.tsapp/[locale]/requirements/[id]/_detail/use-deviation-workflow.tsapp/[locale]/requirements/[id]/_detail/use-specification-item-context.tsapp/[locale]/requirements/[id]/requirement-detail-client.tsxapp/[locale]/requirements/requirements-client.tsxapp/[locale]/specification-item-statuses/page.tsxapp/[locale]/specification-item-statuses/specification-item-statuses-client.tsxapp/[locale]/specifications/[slug]/page.tsxapp/[locale]/specifications/[slug]/reports/layout.tsxapp/[locale]/specifications/[slug]/reports/print/list/page.tsxapp/[locale]/specifications/[slug]/requirements-specification-detail-client.tsxapp/[locale]/specifications/[slug]/specification-edit-panel.tsxapp/[locale]/specifications/implementation-types/implementation-types-client.tsxapp/[locale]/specifications/implementation-types/page.tsxapp/[locale]/specifications/lifecycle-statuses/lifecycle-statuses-client.tsxapp/[locale]/specifications/lifecycle-statuses/page.tsxapp/[locale]/specifications/page.tsxapp/[locale]/specifications/responsibility-areas/page.tsxapp/[locale]/specifications/responsibility-areas/responsibility-areas-client.tsxapp/[locale]/specifications/specifications-client.tsxapp/api/specification-implementation-types/[id]/route.tsapp/api/specification-implementation-types/route.tsapp/api/specification-item-deviations/[itemId]/route.tsapp/api/specification-item-statuses/[id]/route.tsapp/api/specification-item-statuses/route.tsapp/api/specification-lifecycle-statuses/[id]/route.tsapp/api/specification-lifecycle-statuses/route.tsapp/api/specification-local-deviations/[id]/decision/route.tsapp/api/specification-local-deviations/[id]/request-review/route.tsapp/api/specification-local-deviations/[id]/revert-to-draft/route.tsapp/api/specification-local-deviations/[id]/route.tsapp/api/specification-responsibility-areas/[id]/route.tsapp/api/specification-responsibility-areas/route.tsapp/api/specifications/[id]/deviations/route.tsapp/api/specifications/[id]/items/[itemId]/route.tsapp/api/specifications/[id]/items/route.tsapp/api/specifications/[id]/local-requirements/[localRequirementId]/route.tsapp/api/specifications/[id]/local-requirements/route.tsapp/api/specifications/[id]/needs-references/route.tsapp/api/specifications/[id]/report-items/route.tsapp/api/specifications/[id]/route.tsapp/api/specifications/route.tscomponents/Navigation.tsxcomponents/RequirementsTable.tsxcomponents/SpecificationLocalRequirementDetailClient.tsxcomponents/SpecificationLocalRequirementForm.tsxcomponents/_requirements-table/SpecificationItemStatusSelect.tsxcomponents/reports/pdf/PdfReportRenderer.tsxcomponents/reports/print/PrintReportRenderer.tsxcspell.jsoncdocs/arkitekturbeskrivning-kravhantering.mddocs/database-schema.mddocs/developer-mode-overlay.mddocs/dogfood-seed.mddocs/guide/README.mddocs/lifecycle-workflow.mddocs/mcp-server-contributor-guide.mddocs/mcp-server-user-guide.mddocs/reference-data-and-ai.mddocs/reports.mddocs/requirements-ui-behaviour.mdlib/dal/deviations.tslib/dal/package-implementation-types.tslib/dal/package-responsibility-areas.tslib/dal/requirements-specifications.tslib/dal/requirements.tslib/dal/specification-implementation-types.tslib/dal/specification-item-statuses.tslib/dal/specification-lifecycle-statuses.tslib/dal/specification-responsibility-areas.tslib/mcp/server.tslib/reports/data/fetch-deviation.tslib/reports/data/fetch-specification-items.tslib/reports/templates/deviation-review-template.tslib/reports/templates/list-template.tslib/reports/types.tslib/requirements/auth.tslib/requirements/list-view.tslib/requirements/service.tslib/requirements/types.tslib/slug.tslib/specification-item-status-constants.tslib/typeorm/entities/deviation.tslib/typeorm/entities/index.tslib/typeorm/entities/package-implementation-type.tslib/typeorm/entities/package-lifecycle-status.tslib/typeorm/entities/package-local-requirement-norm-reference.tslib/typeorm/entities/package-local-requirement-usage-scenario.tslib/typeorm/entities/package-needs-reference.tslib/typeorm/entities/package-responsibility-area.tslib/typeorm/entities/requirements-specification-item.tslib/typeorm/entities/requirements-specification.tslib/typeorm/entities/specification-implementation-type.tslib/typeorm/entities/specification-item-status.tslib/typeorm/entities/specification-lifecycle-status.tslib/typeorm/entities/specification-local-requirement-deviation.tslib/typeorm/entities/specification-local-requirement-norm-reference.tslib/typeorm/entities/specification-local-requirement-usage-scenario.tslib/typeorm/entities/specification-local-requirement.tslib/typeorm/entities/specification-needs-reference.tslib/typeorm/entities/specification-responsibility-area.tslib/ui-terminology.tsmessages/en.jsonmessages/sv.jsontests/quality/QUALITY.mdtests/quality/functional.test.tstests/unit/admin-client.test.tsxtests/unit/admin-requirement-columns-route.test.tstests/unit/deviations-dal.test.tstests/unit/dogfood-seed.test.tstests/unit/edit-requirement-client.test.tsxtests/unit/implementation-types-client.test.tsxtests/unit/lifecycle-statuses-client.test.tsxtests/unit/mcp-http.test.tstests/unit/navigation.test.tsxtests/unit/reference-data-developer-mode.test.tsxtests/unit/requirement-action-rail.test.tsxtests/unit/requirement-detail-client.test.tsxtests/unit/requirement-detail-hooks.test.tsxtests/unit/requirement-list-view.test.tstests/unit/requirement-report-menu.test.tsxtests/unit/requirements-client.test.tsxtests/unit/requirements-dal.test.tstests/unit/requirements-service.test.tstests/unit/requirements-specification-detail-client.test.tsxtests/unit/requirements-specification-item-route.test.tstests/unit/requirements-specification-items-route.test.tstests/unit/requirements-specifications-dal.test.tstests/unit/requirements-table.test.tsxtests/unit/responsibility-areas-client.test.tsxtests/unit/specification-edit-panel.test.tsxtests/unit/specification-item-statuses-client.test.tsxtests/unit/specification-local-deviation-lifecycle-routes.test.tstests/unit/specification-local-requirement-detail-client.test.tsxtests/unit/specification-report-pages.test.tsxtests/unit/specifications-client.test.tsxtests/unit/taxonomy-routes.test.tstypeorm/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 (10)
- app/[locale]/requirement-packages/page.tsx
- lib/typeorm/entities/package-local-requirement-usage-scenario.ts
- lib/typeorm/entities/package-lifecycle-status.ts
- app/[locale]/package-item-statuses/page.tsx
- lib/typeorm/entities/package-responsibility-area.ts
- lib/typeorm/entities/package-needs-reference.ts
- lib/typeorm/entities/package-local-requirement-norm-reference.ts
- lib/dal/package-implementation-types.ts
- lib/dal/package-responsibility-areas.ts
- lib/typeorm/entities/package-implementation-type.ts
…tation and codebase - Updated documentation references to change "package" to "specification" for clarity and consistency. - Modified code comments and variable names to reflect the new terminology. - Adjusted test cases to ensure they align with the updated terminology. - Enhanced SQL queries to include specification-local requirements alongside standard requirements. - Implemented changes in the UI to display specification-related information correctly. - Ensured that all relevant tests pass after the terminology updates.
- Changed navigation path from `/sv/requirement-packages/ETJANSTPLATT` to `/sv/specifications/ETJANST-UPP-2026`. - Updated developer mode trigger from "package reports" to "specification reports". - Adjusted assertions in tests to match new report terminology. test: add integration tests for requirements specification detail - Created `requirements-specification-detail.md` and `requirements-specification-detail.spec.ts` to verify edit functionality and independent scrolling behavior of requirement lists. - Implemented test cases for opening the edit view and ensuring sticky title bars function correctly. test: implement integration tests for specifications list - Added `specifications-list.md` and `specifications-list.spec.ts` to validate rendering of specifications list, filtering functionality, and UI alignment on desktop. test: add unit tests for specification item statuses data access layer - Created `specification-item-statuses-dal.test.ts` to test counting and listing linked package items by status. test: add unit tests for specification local requirement route - Implemented `specification-local-requirement-route.test.ts` to test deletion of specification-local requirements and error handling. test: add unit tests for specifications page components - Created `specifications-page.test.tsx` to ensure correct rendering of specifications metadata and client components. fix: add cascading delete constraint to specification needs references - Created migration `0004_specification_needs_reference_cascade.mjs` to enforce cascading deletes on specification needs references.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
docs/dogfood-seed.md (1)
1-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun markdown lint for this docs change before merge.
This PR modifies a markdown file; please run
npm run lint:mdand include the
result in the PR checks/checklist.As per coding guidelines, “
**/*.md: ... Runnpm run lint:mdwhen markdown
docs change”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dogfood-seed.md` around lines 1 - 123, Run the markdown linter against the modified docs/dogfood-seed.md by executing the project script "npm run lint:md", fix any reported markdown lint errors in docs/dogfood-seed.md (and related .md files if the linter flags them), commit the fixes, and include the lint output in the PR checks (paste the command output into the PR description or attach it as a CI artifact) so the PR shows the lint step passed.lib/mcp/server.ts (4)
1562-1604:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTool title and descriptions still use "package" terminology.
The tool ID was renamed to
requirements_remove_from_specificationbut:
- Line 1563: description says "Identify the package with"
- Line 1582:
requirementIdsdescribes "remove from the package"- Line 1604: title is still "Remove Requirements from Package"
Suggested fixes
description: - 'Unlink one or more requirements from a requirements specification. The requirements themselves are not deleted. Identify the package with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2").', + 'Unlink one or more requirements from a requirements specification. The requirements themselves are not deleted. Identify the specification with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2").', ... requirementIds: z .array(z.number().int().positive()) .min(1) - .describe('Numeric requirement IDs to remove from the package.'), + .describe('Numeric requirement IDs to remove from the specification.'), ... - title: 'Remove Requirements from Package', + title: 'Remove Requirements from Specification',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mcp/server.ts` around lines 1562 - 1604, Update the user-facing strings to use "specification" instead of the old "package" term: in the route/object whose id is requirements_remove_from_specification, change the description text that currently reads "Identify the package with specificationId..." to reference "Identify the specification with specificationId...", update the requirementIds .describe(...) text from "remove from the package" to "remove from the specification", and update the title from "Remove Requirements from Package" to "Remove Requirements from Specification" (also scan nearby example slug text for any remaining "package" mentions and replace similarly).
1476-1528:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTool title and descriptions still use "package" terminology.
The tool ID was renamed to
requirements_add_to_specificationbut:
- Line 1477: description says "Identify the package with"
- Line 1503:
requirementIdsdescribes "to add to the package"- Line 1528: title is still "Add Requirements to Package"
Suggested fixes
description: - 'Link one or more requirements to a requirements specification. Requirements must have a published version; those without are skipped and returned in skippedIds. Optionally attach a needs reference text to all added items. Identify the package with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2").', + 'Link one or more requirements to a requirements specification. Requirements must have a published version; those without are skipped and returned in skippedIds. Optionally attach a needs reference text to all added items. Identify the specification with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2").', ... requirementIds: z .array(z.number().int().positive()) .min(1) .describe( - 'Numeric requirement IDs (not uniqueId strings) to add to the package.', + 'Numeric requirement IDs (not uniqueId strings) to add to the specification.', ), ... - title: 'Add Requirements to Package', + title: 'Add Requirements to Specification',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mcp/server.ts` around lines 1476 - 1528, Update the wording that still uses "package" to "specification" to match the renamed tool id requirements_add_to_specification: change the description field text (symbol: description) that reads "Identify the package with specificationId..." to reference "specification", update the requirementIds input description (symbol: requirementIds) from "to add to the package" to "to add to the specification", and update the title (symbol: title) from "Add Requirements to Package" to "Add Requirements to Specification" so all user-facing strings align with requirements_add_to_specification.
1385-1443:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTool title and description still use "package" terminology.
The tool ID was renamed to
requirements_get_specification_itemsbut:
- Line 1386: description says "Identify the package with specificationId"
- Line 1443: title is still "Get Package Items"
This creates a mismatch between the tool ID and its title/description.
Suggested fixes
description: - 'List requirements (krav) linked to a specific requirements specification, with optional description search. Identify the package with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2") from requirements_list_specifications.', + 'List requirements (krav) linked to a specific requirements specification, with optional description search. Identify the specification with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2") from requirements_list_specifications.', ... - title: 'Get Package Items', + title: 'Get Specification Items',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mcp/server.ts` around lines 1385 - 1443, The tool definition for requirements_get_specification_items uses "package" in its description and title; update the description string (near inputSchema.description) to say "Identify the requirements specification with specificationId..." (or "Identify the specification by specificationId or specificationSlug") and change the title from "Get Package Items" to "Get Specification Items" (or "Get Requirements Specification Items") so wording matches the tool ID and outputSchema.specificationId; ensure any other user-facing strings in this object (e.g., the top-level description and title properties) replace "package" with "specification" for consistency.
1318-1354:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncomplete terminology rename in tool descriptions and schema.
The tool ID and title were updated to use "specification" but several descriptions still reference "package":
- Line 1319: description ends with "for each package"
- Line 1327:
nameSearchdescribes "Swedish and English package names"- Line 1335: output schema field is still named
packagesPer coding guidelines, MCP tool descriptions and schema field descriptions should be aligned with the tool behavior. MCP clients will see inconsistent terminology.
Suggested terminology fixes
description: - 'List all requirements specifications, optionally filtered by name. Returns id, uniqueId (slug), names, item count, responsibility area, and implementation type for each package.', + 'List all requirements specifications, optionally filtered by name. Returns id, uniqueId (slug), names, item count, responsibility area, and implementation type for each specification.', inputSchema: z .object({ locale: z.enum(['en', 'sv']).default('en'), nameSearch: z .string() .optional() .describe( - 'Case-insensitive substring filter applied to both Swedish and English package names.', + 'Case-insensitive substring filter applied to both Swedish and English specification names.', ), responseFormat: z.enum(['json', 'markdown']).default('markdown'), }) .strict(), outputSchema: z .object({ message: z.string(), - packages: z.array( + specifications: z.array(Note: If the output schema field name
packagesmust match the service response shape, consider documenting this in the description or updating the service layer for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mcp/server.ts` around lines 1318 - 1354, Update the terminology to consistently use "specification" instead of "package": edit the description string (symbol: description) to end with "for each specification", change the nameSearch.describe text (symbol: nameSearch) to "Case-insensitive substring filter applied to both Swedish and English specification names", and either rename the output array field packages to specifications in outputSchema (symbol: outputSchema -> packages) or, if renaming breaks the service contract, update the outputSchema description to explicitly document that the field is named "packages" for compatibility while referring to its items as specifications; also ensure the title remains "List Requirements Specifications".tests/unit/requirements-specification-items-route.test.ts (1)
46-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
describeblock label still uses old "requirement-packages" terminology.The suite string should be updated to reflect the renamed route (e.g.,
'specifications/[id]/items route'), consistent with the rename intent of this PR.✏️ Proposed fix
-describe('requirement-packages/[id]/items route', () => { +describe('specifications/[id]/items route', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/requirements-specification-items-route.test.ts` at line 46, Update the top-level test suite label string used in the describe block currently written as "requirement-packages/[id]/items route" to the new route name "specifications/[id]/items route" so the test description matches the renamed route; locate the describe call (describe('requirement-packages/[id]/items route', ...) in tests/unit/requirements-specification-items-route.test.ts) and replace the string, then run the unit tests to confirm no regressions.app/[locale]/specifications/[slug]/requirements-specification-detail-client.tsx (1)
680-716:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid rolling back the whole list on one PATCH failure.
Line 710 / Line 713 restore the full captured
specificationItemssnapshot. If two status changes are made back-to-back, a late failure from the first request can wipe out the second successful update as well. Re-fetch the list on failure, or revert only the touched row.Suggested fix
- const prev = specificationItems // Optimistic update setPackageItems(prev => prev.map(item => { if (item.itemRef !== itemRef) return item @@ const res = await apiFetch( `/api/specifications/${pkg.id}/items/${encodeURIComponent(itemRef)}`, { method: 'PATCH', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ specificationItemStatusId: statusId }), }, ) if (!res.ok) { - setPackageItems(prev) + await fetchPackageItems({ throwOnError: true }) } } catch { - setPackageItems(prev) + await fetchPackageItems({ throwOnError: true }) } }, - [pkg, specificationItemStatuses, specificationItems], + [fetchPackageItems, pkg, specificationItemStatuses, specificationItems], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[locale]/specifications/[slug]/requirements-specification-detail-client.tsx around lines 680 - 716, The current optimistic-update in handleSpecificationItemStatusChange captures the entire specificationItems snapshot (prev) and restores it on any PATCH failure, which can overwrite subsequent successful updates; instead on failure either (A) re-fetch the items list (call the existing fetch/loader that populates specificationItems) to get authoritative state, or (B) revert only the single touched row by using the prev snapshot to find the original item for the given itemRef and call setPackageItems(prevState => prevState.map(i => i.itemRef === itemRef ? originalItemFromPrev : i)); update the catch and non-ok branches to perform one of these targeted rollback strategies rather than resetting the whole list.app/[locale]/specifications/specifications-client.tsx (1)
388-441:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
fetchPackages()at Line 435 is a floating promise — addvoid.All other call sites in this file either use
void fetchPackages()(lines 384–385) orawait fetchPackages()(line 495). The bare call on line 435 is inconsistent and will be flagged by@typescript-eslint/no-floating-promisesin a strict-mode TypeScript project.🛠️ Proposed fix
- fetchPackages() + void fetchPackages()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[locale]/specifications/specifications-client.tsx around lines 388 - 441, The handleSubmit function calls fetchPackages() without awaiting or marking the promise as intentionally ignored, causing a floating promise; change that call inside handleSubmit (after setForm(resetForm())) to use void fetchPackages() so linting (`@typescript-eslint/no-floating-promises`) is satisfied and intent is explicit; update the fetchPackages invocation only (do not await or alter surrounding state updates like setShowForm, setEditPkg, setOpenHelp, setSlugEdited, setForm) to preserve existing behavior.
🧹 Nitpick comments (2)
app/[locale]/requirements/[id]/_detail/RequirementActionRail.tsx (1)
7-7: ⚡ Quick winConsider replacing
PackagePluswith a document/specification-oriented icon.
PackagePlus(a box with a plus sign) visually communicates "add to a package," which is now semantically at odds with the renamed "add to specification" action. An icon likeFilePlusorBookPlusfromlucide-reactwould better reinforce the new terminology for users.♻️ Suggested change
import { Archive, Check, Edit, - PackagePlus, + FilePlus, RotateCcw, Share2, Trash2, } from 'lucide-react'- <PackagePlus aria-hidden="true" className="h-4 w-4" /> + <FilePlus aria-hidden="true" className="h-4 w-4" />Also applies to: 192-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[locale]/requirements/[id]/_detail/RequirementActionRail.tsx at line 7, Replace the PackagePlus icon import and its usages in RequirementActionRail.tsx with a document-oriented icon (e.g., FilePlus or BookPlus from lucide-react) to match the "add to specification" action; update the import list to remove PackagePlus and import FilePlus (or BookPlus) instead, then replace the JSX occurrences of <PackagePlus .../> (including the other occurrence referenced) with <FilePlus .../> (or <BookPlus .../>) preserving props and accessibility attributes so visuals and behavior remain unchanged.tests/unit/requirements-table.test.tsx (1)
730-788: ⚡ Quick winAdd coverage for the editable specification-item-status branch.
These cases only exercise the read-only renderer. A regression in the renamed
onSpecificationItemStatusChange+SpecificationItemStatusSelectpath incomponents/RequirementsTable.tsx(Lines 2372-2390) would still pass this suite.As per coding guidelines, "When changing visible UI elements, labels, roles, or layout surfaces, update the relevant unit and integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/requirements-table.test.tsx` around lines 730 - 788, Tests only cover the read-only status renderer; add a unit that exercises the editable branch by rendering RequirementsTable with the specification-item-status column editable, providing a spy for onSpecificationItemStatusChange and asserting that SpecificationItemStatusSelect is rendered and that selecting/changing a status calls onSpecificationItemStatusChange with the expected args; specifically target the renamed handler onSpecificationItemStatusChange and the SpecificationItemStatusSelect component path in RequirementsTable to prevent regressions in lines around the previous 2372-2390 area.
🤖 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]/specifications/[slug]/requirements-specification-detail-client.tsx:
- Around line 1001-1002: Replace the hardcoded English filename assigned to
a.download by resolving it from your translations instead of embedding text;
update the assignment that sets a.download (currently using locale === 'sv' ?
'kravunderlag.csv' : 'requirement-package.csv') to call the app's i18n lookup
(e.g., useTranslations/getDictionary or the existing translation hook) and use a
new translation key such as "specification.downloadFilename" with
language-specific values (sv: "kravunderlag.csv", en: "specification.csv");
ensure you reference the same translation API used elsewhere in this component
so visible labels (a.download) are not hardcoded.
In `@docs/dogfood-seed.md`:
- Around line 26-37: Reformat the ID ranges markdown table (the block starting
with the header "| Entity | Base IDs | Dogfood IDs |") to MD060 compact style by
removing the padding spaces around cell content and pipes (e.g., change "|
`owners` | 1–3 | 1001–1005 |" to
"|`owners`|1–3|1001–1005|"), keep the header and separator row intact (using the
same number of columns and backticks for identifiers) and preserve all cell
text/IDs unchanged except for whitespace so the table becomes unpadded/compact.
In `@lib/dal/specification-item-statuses.ts`:
- Around line 197-201: The deleteSpecificationItemStatus function currently
deletes any id; protect seeded system statuses by checking if id equals
DEFAULT_SPECIFICATION_ITEM_STATUS_ID or DEVIATED_SPECIFICATION_ITEM_STATUS_ID
and refuse to delete them (throw a descriptive error or return without
deleting). Modify deleteSpecificationItemStatus (in
lib/dal/specification-item-statuses.ts) to perform this guard before calling
db.getRepository(specificationItemStatusEntity).delete(id) so seeded statuses
cannot be removed.
In `@tests/quality/functional.test.ts`:
- Line 785: Multiple it() blocks share the exact title "Scenario 12: concurrent
archiving attempts are atomic and strictly targeted", preventing vitest -t from
selecting individual sub-scenarios; update each it() description in
tests/quality/functional.test.ts (the four it() occurrences that test
initiateArchiving, approveArchiving, approve vs cancel, and manual state
manipulation) to have unique, verbatim titles such as "Scenario 12a: concurrent
initiateArchiving attempts are atomic and strictly targeted", "Scenario 12b:
concurrent approveArchiving attempts are atomic and strictly targeted",
"Scenario 12c: concurrent approveArchiving vs cancelArchiving are atomic and
strictly targeted", and "Scenario 12d: strict-target behavior with manual state
manipulation", respectively; ensure the new titles match any corresponding
headings in QUALITY.md (or update QUALITY.md to reflect 12a–12d) so vitest -t
can target each scenario individually.
In `@tests/unit/taxonomy-routes.test.ts`:
- Around line 125-133: The test imports for route handlers (GET, POST, PUT,
DELETE) from '@/app/api/catalog/specification-item-statuses/route' and
'@/app/api/catalog/specification-item-statuses/[id]/route' are failing TS2307
because those named exports don't exist at those paths; either add and export
the corresponding handler functions (GET, POST in
specification-item-statuses/route and GET, PUT, DELETE in
specification-item-statuses/[id]/route) with the exact names used in the test,
or update the test imports to point to the actual module that exports these
handlers—ensure the exported function names match the test (GET, POST, PUT,
DELETE) so TypeScript can resolve them.
---
Outside diff comments:
In
`@app/`[locale]/specifications/[slug]/requirements-specification-detail-client.tsx:
- Around line 680-716: The current optimistic-update in
handleSpecificationItemStatusChange captures the entire specificationItems
snapshot (prev) and restores it on any PATCH failure, which can overwrite
subsequent successful updates; instead on failure either (A) re-fetch the items
list (call the existing fetch/loader that populates specificationItems) to get
authoritative state, or (B) revert only the single touched row by using the prev
snapshot to find the original item for the given itemRef and call
setPackageItems(prevState => prevState.map(i => i.itemRef === itemRef ?
originalItemFromPrev : i)); update the catch and non-ok branches to perform one
of these targeted rollback strategies rather than resetting the whole list.
In `@app/`[locale]/specifications/specifications-client.tsx:
- Around line 388-441: The handleSubmit function calls fetchPackages() without
awaiting or marking the promise as intentionally ignored, causing a floating
promise; change that call inside handleSubmit (after setForm(resetForm())) to
use void fetchPackages() so linting (`@typescript-eslint/no-floating-promises`) is
satisfied and intent is explicit; update the fetchPackages invocation only (do
not await or alter surrounding state updates like setShowForm, setEditPkg,
setOpenHelp, setSlugEdited, setForm) to preserve existing behavior.
In `@docs/dogfood-seed.md`:
- Around line 1-123: Run the markdown linter against the modified
docs/dogfood-seed.md by executing the project script "npm run lint:md", fix any
reported markdown lint errors in docs/dogfood-seed.md (and related .md files if
the linter flags them), commit the fixes, and include the lint output in the PR
checks (paste the command output into the PR description or attach it as a CI
artifact) so the PR shows the lint step passed.
In `@lib/mcp/server.ts`:
- Around line 1562-1604: Update the user-facing strings to use "specification"
instead of the old "package" term: in the route/object whose id is
requirements_remove_from_specification, change the description text that
currently reads "Identify the package with specificationId..." to reference
"Identify the specification with specificationId...", update the requirementIds
.describe(...) text from "remove from the package" to "remove from the
specification", and update the title from "Remove Requirements from Package" to
"Remove Requirements from Specification" (also scan nearby example slug text for
any remaining "package" mentions and replace similarly).
- Around line 1476-1528: Update the wording that still uses "package" to
"specification" to match the renamed tool id requirements_add_to_specification:
change the description field text (symbol: description) that reads "Identify the
package with specificationId..." to reference "specification", update the
requirementIds input description (symbol: requirementIds) from "to add to the
package" to "to add to the specification", and update the title (symbol: title)
from "Add Requirements to Package" to "Add Requirements to Specification" so all
user-facing strings align with requirements_add_to_specification.
- Around line 1385-1443: The tool definition for
requirements_get_specification_items uses "package" in its description and
title; update the description string (near inputSchema.description) to say
"Identify the requirements specification with specificationId..." (or "Identify
the specification by specificationId or specificationSlug") and change the title
from "Get Package Items" to "Get Specification Items" (or "Get Requirements
Specification Items") so wording matches the tool ID and
outputSchema.specificationId; ensure any other user-facing strings in this
object (e.g., the top-level description and title properties) replace "package"
with "specification" for consistency.
- Around line 1318-1354: Update the terminology to consistently use
"specification" instead of "package": edit the description string (symbol:
description) to end with "for each specification", change the
nameSearch.describe text (symbol: nameSearch) to "Case-insensitive substring
filter applied to both Swedish and English specification names", and either
rename the output array field packages to specifications in outputSchema
(symbol: outputSchema -> packages) or, if renaming breaks the service contract,
update the outputSchema description to explicitly document that the field is
named "packages" for compatibility while referring to its items as
specifications; also ensure the title remains "List Requirements
Specifications".
In `@tests/unit/requirements-specification-items-route.test.ts`:
- Line 46: Update the top-level test suite label string used in the describe
block currently written as "requirement-packages/[id]/items route" to the new
route name "specifications/[id]/items route" so the test description matches the
renamed route; locate the describe call
(describe('requirement-packages/[id]/items route', ...) in
tests/unit/requirements-specification-items-route.test.ts) and replace the
string, then run the unit tests to confirm no regressions.
---
Nitpick comments:
In `@app/`[locale]/requirements/[id]/_detail/RequirementActionRail.tsx:
- Line 7: Replace the PackagePlus icon import and its usages in
RequirementActionRail.tsx with a document-oriented icon (e.g., FilePlus or
BookPlus from lucide-react) to match the "add to specification" action; update
the import list to remove PackagePlus and import FilePlus (or BookPlus) instead,
then replace the JSX occurrences of <PackagePlus .../> (including the other
occurrence referenced) with <FilePlus .../> (or <BookPlus .../>) preserving
props and accessibility attributes so visuals and behavior remain unchanged.
In `@tests/unit/requirements-table.test.tsx`:
- Around line 730-788: Tests only cover the read-only status renderer; add a
unit that exercises the editable branch by rendering RequirementsTable with the
specification-item-status column editable, providing a spy for
onSpecificationItemStatusChange and asserting that SpecificationItemStatusSelect
is rendered and that selecting/changing a status calls
onSpecificationItemStatusChange with the expected args; specifically target the
renamed handler onSpecificationItemStatusChange and the
SpecificationItemStatusSelect component path in RequirementsTable to prevent
regressions in lines around the previous 2372-2390 area.
🪄 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: 3bb3ccf6-08d6-4893-b4b5-91c1782b7cd2
📒 Files selected for processing (36)
app/[locale]/requirements/[id]/_detail/RequirementActionRail.tsxapp/[locale]/specification-item-statuses/specification-item-statuses-client.tsxapp/[locale]/specifications/[slug]/requirements-specification-detail-client.tsxapp/[locale]/specifications/[slug]/specification-edit-panel.tsxapp/[locale]/specifications/specifications-client.tsxapp/api/specification-responsibility-areas/[id]/route.tsapp/api/specifications/[id]/items/[itemId]/route.tsapp/api/specifications/[id]/items/route.tsapp/api/specifications/[id]/local-requirements/[localRequirementId]/route.tscomponents/RequirementsTable.tsxcomponents/SpecificationLocalRequirementDetailClient.tsxcomponents/reports/pdf/PdfReportRenderer.tsxcspell.jsoncdocs/database-schema.mddocs/developer-mode-overlay.mddocs/dogfood-seed.mddocs/reference-data-and-ai.mddocs/reports.mdlib/dal/specification-item-statuses.tslib/mcp/server.tslib/requirements/service.tslib/specification-item-status-constants.tslib/typeorm/entities/specification-needs-reference.tstests/quality/QUALITY.mdtests/quality/functional.test.tstests/unit/requirement-detail-client.test.tsxtests/unit/requirements-service.test.tstests/unit/requirements-specification-detail-client.test.tsxtests/unit/requirements-specification-item-route.test.tstests/unit/requirements-specification-items-route.test.tstests/unit/requirements-table.test.tsxtests/unit/specification-edit-panel.test.tsxtests/unit/specification-item-statuses-client.test.tsxtests/unit/specifications-client.test.tsxtests/unit/taxonomy-routes.test.tstypeorm/seed.mjs
✅ Files skipped from review due to trivial changes (6)
- lib/specification-item-status-constants.ts
- docs/developer-mode-overlay.md
- docs/reports.md
- docs/database-schema.md
- tests/unit/requirements-service.test.ts
- tests/quality/QUALITY.md
🚧 Files skipped from review as they are similar to previous changes (14)
- lib/typeorm/entities/specification-needs-reference.ts
- app/api/specifications/[id]/local-requirements/[localRequirementId]/route.ts
- tests/unit/specification-item-statuses-client.test.tsx
- tests/unit/requirements-specification-item-route.test.ts
- app/api/specifications/[id]/items/route.ts
- cspell.jsonc
- app/api/specification-responsibility-areas/[id]/route.ts
- tests/unit/requirements-specification-detail-client.test.tsx
- tests/unit/specification-edit-panel.test.tsx
- app/api/specifications/[id]/items/[itemId]/route.ts
- components/SpecificationLocalRequirementDetailClient.tsx
- app/[locale]/specifications/[slug]/specification-edit-panel.tsx
- lib/requirements/service.ts
- tests/unit/requirement-detail-client.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/skills/run-spec-audit/references/integration-contracts.md (1)
65-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Package Detailcontract appears to document a non-existent field.Line 76 lists
localRequirementNextSequence, but the current package-detail DAL mapping (lib/dal/requirements-specifications.ts:433-457and:379-430) does not expose that field. This makes the contract table inaccurate as audit ground truth.Proposed fix
-| `localRequirementNextSequence` | `number` | Next local seq |If the API should return it, implement and map it in
app/api/specifications/[id]/route.tsand DAL, then keep the row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/run-spec-audit/references/integration-contracts.md around lines 65 - 77, The contract lists a non-existent field localRequirementNextSequence; either remove that row from the contract or implement and surface the field: update the DAL in lib/dal/requirements-specifications.ts (the package-detail assembly functions) to compute localRequirementNextSequence (e.g., max local requirement sequence for the package + 1) and include it on the package DTO, then update app/api/specifications/[id]/route.ts to map the returned DAL package object to include localRequirementNextSequence in the JSON response so the REST contract matches the actual API output.tests/integration/specifications-list.spec.ts (1)
9-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale test
describeandtestnames to match renamed domain.The outer describe block (Line 9) still says
"Requirement packages list filter"and the inner test (Line 12) still says"filters the table by package name". Both should reflect the specifications terminology; failing CI output will show misleading labels.✏️ Proposed fix
- test.describe(`Requirement packages list filter — ${viewport.name} (${viewport.width}×${viewport.height})`, () => { + test.describe(`Requirements specifications list filter — ${viewport.name} (${viewport.width}×${viewport.height})`, () => { test.use({ viewport: { width: viewport.width, height: viewport.height } }) - test('filters the table by package name and clears the search', async ({ + test('filters the table by specification name and clears the search', async ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/specifications-list.spec.ts` around lines 9 - 12, Update the stale human-readable test labels: change the test.describe string "Requirement packages list filter — ${viewport.name} (...)" to use the specifications domain wording (e.g., "Specifications list filter — ${viewport.name} (...)"), and rename the inner test title passed to test(...) from "filters the table by package name and clears the search" to match the specifications terminology (e.g., "filters the table by specification name and clears the search"); modify only the description strings in the test.describe(...) and test(...) calls so CI output and test reports reflect the renamed domain.tests/integration/developer-mode-overlay.md (1)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncomplete rename: stale "package" terminology in flowchart, section headings, and intro
The following locations still use old "package" terminology after the rename to "specification reports":
Location Stale text Expected Line 9 package-context report controlsspecification report controlsLine 26 C -- package reports --> L[...]C -- specification reports --> L[...]Lines 27–29 Hover package report control,Assert package report chipspecification report control/chipLine 134 ## exposes package report controls in developer mode## exposes specification report controls in developer modeLines 136, 143 ### Purpose: Package Report Reference/### Step-by-Step Flow: Package Report ReferenceSpecification Report ReferenceAlso applies to: 26-29, 134-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/developer-mode-overlay.md` at line 9, Replace all remaining uses of "package" terminology with "specification" in this markdown: change "package-context report controls" to "specification report controls", update the flowchart arrow label `C -- package reports --> L[...]` to `C -- specification reports --> L[...]`, rename occurrences "Hover package report control" and "Assert package report chip" to "Hover specification report control" and "Assert specification report chip", and update the section heading `## exposes package report controls in developer mode` and the subheadings `### Purpose: Package Report Reference` / `### Step-by-Step Flow: Package Report Reference` to use "Specification Report Reference"; search for the listed phrases to ensure no stale instances remain.
🧹 Nitpick comments (4)
.github/skills/run-spec-audit/references/integration-contracts.md (1)
65-67: ⚡ Quick winUse “Specification” terminology in section titles for consistency.
Line 65 and Line 85 still use “Package …” while the routes and fields are specification-scoped. Renaming these headers improves clarity and aligns with this PR’s stated terminology migration.
Proposed fix
-## REST: Package Detail `/api/specifications/[id]` +## REST: Specification Detail `/api/specifications/[id]` -## REST: Package Items `/api/specifications/[id]/items` +## REST: Specification Items `/api/specifications/[id]/items`Also applies to: 85-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/run-spec-audit/references/integration-contracts.md around lines 65 - 67, Update the section headers to use "Specification" terminology for consistency: rename the header "REST: Package Detail `/api/specifications/[id]`" to "REST: Specification Detail `/api/specifications/[id]`" (and likewise change the header at lines 85–87 from "Package ..." to "Specification ..." to match the route and fields). Locate these markdown headings in integration-contracts.md and replace "Package" with "Specification" so the titles align with the `app/api/specifications/[id]/route.ts` and the overall terminology migration.tests/unit/specification-local-requirement-route.test.ts (1)
31-69: ⚡ Quick winExpand coverage to the remaining response branches.
The entire mock infrastructure is already in place, but only the
500error path is exercised. The route handler (Context snippet 1) has four other reachable branches that are currently untested:
Scenario Expected response localRequirementIdis non-numeric / < 1400 { error: 'Invalid localRequirementId' }resolvePackageIdreturnsnull(unknown slug)404 { error: 'Not found' }deleteSpecificationLocalRequirementresolvesfalse(row missing)404 { error: 'Not found' }deleteSpecificationLocalRequirementresolvestrue200 { ok: true }Adding these cases is low-cost because
mockDb,makeParams, and all mock stubs are already wired up.🧪 Suggested additional test cases
+ it('returns 400 for a non-integer localRequirementId', async () => { + const response = await DELETE( + new NextRequest('http://localhost/api/specifications/pkg/local-requirements/abc'), + makeParams('pkg', 'abc'), + ) + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ error: 'Invalid localRequirementId' }) + }) + + it('returns 404 when the specification slug is not found', async () => { + mocks.getPackageBySlug.mockResolvedValue(null) + const response = await DELETE( + new NextRequest('http://localhost/api/specifications/unknown/local-requirements/41'), + makeParams('unknown', '41'), + ) + expect(response.status).toBe(404) + await expect(response.json()).resolves.toEqual({ error: 'Not found' }) + }) + + it('returns 404 when the local requirement row does not exist', async () => { + mocks.deleteSpecificationLocalRequirement.mockResolvedValue(false) + const response = await DELETE( + new NextRequest('http://localhost/api/specifications/pkg/local-requirements/41'), + makeParams('pkg', '41'), + ) + expect(response.status).toBe(404) + await expect(response.json()).resolves.toEqual({ error: 'Not found' }) + }) + + it('returns 200 on successful deletion', async () => { + mocks.deleteSpecificationLocalRequirement.mockResolvedValue(true) + const response = await DELETE( + new NextRequest('http://localhost/api/specifications/pkg/local-requirements/41'), + makeParams('pkg', '41'), + ) + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ ok: true }) + })As per coding guidelines, test files matching
**/*.test.{mjs,js,ts}should "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/specification-local-requirement-route.test.ts` around lines 31 - 69, Add unit tests for the remaining response branches of the DELETE route: create separate it blocks that call the DELETE handler (same call pattern as the existing test using NextRequest and makeParams) and assert the expected status and JSON for each case—(1) non-numeric or <1 localRequirementId: set makeParams('pkg','0' or 'abc') and expect 400 with { error: 'Invalid localRequirementId' }, (2) resolvePackageId returns null/unknown slug: mock getPackageBySlug (or resolvePackageId) to return null and expect 404 with { error: 'Not found' }, (3) deleteSpecificationLocalRequirement resolves false: mock it to resolve false and expect 404 with { error: 'Not found' }, and (4) resolves true: mock it to resolve true and expect 200 with { ok: true }; reuse mockDb, makeParams, and existing mocks, and mirror the pattern used in the existing 500 test (including spying on console.error only where needed).app/api/catalog/specification-item-statuses/route.ts (2)
4-8: ⚡ Quick winQUALITY.md classification required for specification-item-status DAL switch.
Switching GET/POST from package-item-status DAL to specification-item-status DAL changes which database tables are queried — this is outward-facing behavior change. Per coding guidelines for
lib/dal/**/*.tsandapp/api/**/route.ts, this must be classified:
- If this is a pure refactor with no behavior change → no update needed.
- If it's new/changed outward behavior → add/update the matching Fitness Scenario in
tests/quality/QUALITY.mdandtests/quality/functional.test.ts.As per coding guidelines: "Read
tests/quality/QUALITY.mdbefore changing lifecycle, package-item status, MCP tools, report columns, or admin-default behavior" and classify the change accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/catalog/specification-item-statuses/route.ts` around lines 4 - 8, You switched GET/POST handlers to use createSpecificationItemStatus and listSpecificationItemStatuses (and DEVIATED_SPECIFICATION_ITEM_STATUS_ID), which may change outward-facing DB behavior; determine whether this is purely a refactor or a behavior change and update tests/quality accordingly: if behavior changed, add or update the corresponding Fitness Scenario entry in QUALITY.md and add/adjust the matching test in tests/quality/functional.test.ts to cover the new specification-item-status table behavior (include the new status id DEVIATED_SPECIFICATION_ITEM_STATUS_ID in test fixtures/assertions); if it is purely internal refactor, add a short note in QUALITY.md marking it as refactor-only. Ensure the modifications reference createSpecificationItemStatus, listSpecificationItemStatuses, and DEVIATED_SPECIFICATION_ITEM_STATUS_ID so reviewers can verify coverage.
3-3: Naming inconsistency:countLinkedPackageItemsis semantically correct but misleadingly named.The function
countLinkedPackageItems(lines 3, 14) correctly queriesrequirements_specification_itemsandspecification_local_requirementsbyspecification_item_status_id, not package-item tables. The logic is sound—no data correctness issue exists. However, the function name suggests it counts package items, creating confusion in a specification-item-status context. Consider renaming tocountLinkedSpecificationItemsfor semantic clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/catalog/specification-item-statuses/route.ts` at line 3, Rename the function countLinkedPackageItems to countLinkedSpecificationItems to match its behavior (it queries requirements_specification_items and specification_local_requirements by specification_item_status_id); update the function declaration, all references/imports/exports (e.g., any uses within route handlers or tests) to the new name, and update any JSDoc/comments to reflect "specification items" instead of "package items" so the symbol and documentation are semantically consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/developer-mode-overlay.md`:
- Around line 144-151: Docs and test disagree: the markdown step currently
expects "report print button: package reports" but
developer-mode-overlay.spec.ts asserts 'report print button: specification
reports'; update the documentation to match the test by changing the chip
assertion in tests/integration/developer-mode-overlay.md (the step that
references data-developer-mode-value and the chip text) to "report print button:
specification reports" so it aligns with the assertion in
developer-mode-overlay.spec.ts and the data-developer-mode-value="specification
reports" selector.
In `@tests/unit/specifications-page.test.tsx`:
- Around line 35-50: Rename the exported async component function
KravunderlagDetailPage to RequirementsSpecificationDetailPage in the page module
and update the test to import and call RequirementsSpecificationDetailPage (also
update the it description string to mention
RequirementsSpecificationDetailClient instead of the old Swedish name); ensure
the test's destructured variable and the params call remain the same except for
the new function name so the rendered output still contains the slug value used
in the expectation.
---
Outside diff comments:
In @.github/skills/run-spec-audit/references/integration-contracts.md:
- Around line 65-77: The contract lists a non-existent field
localRequirementNextSequence; either remove that row from the contract or
implement and surface the field: update the DAL in
lib/dal/requirements-specifications.ts (the package-detail assembly functions)
to compute localRequirementNextSequence (e.g., max local requirement sequence
for the package + 1) and include it on the package DTO, then update
app/api/specifications/[id]/route.ts to map the returned DAL package object to
include localRequirementNextSequence in the JSON response so the REST contract
matches the actual API output.
In `@tests/integration/developer-mode-overlay.md`:
- Line 9: Replace all remaining uses of "package" terminology with
"specification" in this markdown: change "package-context report controls" to
"specification report controls", update the flowchart arrow label `C -- package
reports --> L[...]` to `C -- specification reports --> L[...]`, rename
occurrences "Hover package report control" and "Assert package report chip" to
"Hover specification report control" and "Assert specification report chip", and
update the section heading `## exposes package report controls in developer
mode` and the subheadings `### Purpose: Package Report Reference` / `###
Step-by-Step Flow: Package Report Reference` to use "Specification Report
Reference"; search for the listed phrases to ensure no stale instances remain.
In `@tests/integration/specifications-list.spec.ts`:
- Around line 9-12: Update the stale human-readable test labels: change the
test.describe string "Requirement packages list filter — ${viewport.name} (...)"
to use the specifications domain wording (e.g., "Specifications list filter —
${viewport.name} (...)"), and rename the inner test title passed to test(...)
from "filters the table by package name and clears the search" to match the
specifications terminology (e.g., "filters the table by specification name and
clears the search"); modify only the description strings in the
test.describe(...) and test(...) calls so CI output and test reports reflect the
renamed domain.
---
Nitpick comments:
In @.github/skills/run-spec-audit/references/integration-contracts.md:
- Around line 65-67: Update the section headers to use "Specification"
terminology for consistency: rename the header "REST: Package Detail
`/api/specifications/[id]`" to "REST: Specification Detail
`/api/specifications/[id]`" (and likewise change the header at lines 85–87 from
"Package ..." to "Specification ..." to match the route and fields). Locate
these markdown headings in integration-contracts.md and replace "Package" with
"Specification" so the titles align with the
`app/api/specifications/[id]/route.ts` and the overall terminology migration.
In `@app/api/catalog/specification-item-statuses/route.ts`:
- Around line 4-8: You switched GET/POST handlers to use
createSpecificationItemStatus and listSpecificationItemStatuses (and
DEVIATED_SPECIFICATION_ITEM_STATUS_ID), which may change outward-facing DB
behavior; determine whether this is purely a refactor or a behavior change and
update tests/quality accordingly: if behavior changed, add or update the
corresponding Fitness Scenario entry in QUALITY.md and add/adjust the matching
test in tests/quality/functional.test.ts to cover the new
specification-item-status table behavior (include the new status id
DEVIATED_SPECIFICATION_ITEM_STATUS_ID in test fixtures/assertions); if it is
purely internal refactor, add a short note in QUALITY.md marking it as
refactor-only. Ensure the modifications reference createSpecificationItemStatus,
listSpecificationItemStatuses, and DEVIATED_SPECIFICATION_ITEM_STATUS_ID so
reviewers can verify coverage.
- Line 3: Rename the function countLinkedPackageItems to
countLinkedSpecificationItems to match its behavior (it queries
requirements_specification_items and specification_local_requirements by
specification_item_status_id); update the function declaration, all
references/imports/exports (e.g., any uses within route handlers or tests) to
the new name, and update any JSDoc/comments to reflect "specification items"
instead of "package items" so the symbol and documentation are semantically
consistent.
In `@tests/unit/specification-local-requirement-route.test.ts`:
- Around line 31-69: Add unit tests for the remaining response branches of the
DELETE route: create separate it blocks that call the DELETE handler (same call
pattern as the existing test using NextRequest and makeParams) and assert the
expected status and JSON for each case—(1) non-numeric or <1 localRequirementId:
set makeParams('pkg','0' or 'abc') and expect 400 with { error: 'Invalid
localRequirementId' }, (2) resolvePackageId returns null/unknown slug: mock
getPackageBySlug (or resolvePackageId) to return null and expect 404 with {
error: 'Not found' }, (3) deleteSpecificationLocalRequirement resolves false:
mock it to resolve false and expect 404 with { error: 'Not found' }, and (4)
resolves true: mock it to resolve true and expect 200 with { ok: true }; reuse
mockDb, makeParams, and existing mocks, and mirror the pattern used in the
existing 500 test (including spying on console.error only where needed).
🪄 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: e77f73bd-dd27-46b4-9329-e98b681ab46a
📒 Files selected for processing (18)
.github/instructions/next-runtime.instructions.md.github/skills/run-spec-audit/references/integration-contracts.md.github/skills/run-spec-audit/references/scrutiny-areas.md.github/skills/update-guide-spec/SKILL.md.markdownlint-cli2.jsoncapp/api/catalog/specification-item-statuses/[id]/route.tsapp/api/catalog/specification-item-statuses/route.tstests/guide/generate-guide.spec.tstests/integration/developer-mode-overlay.mdtests/integration/developer-mode-overlay.spec.tstests/integration/requirements-specification-detail.mdtests/integration/requirements-specification-detail.spec.tstests/integration/specifications-list.mdtests/integration/specifications-list.spec.tstests/unit/specification-item-statuses-dal.test.tstests/unit/specification-local-requirement-route.test.tstests/unit/specifications-page.test.tsxtypeorm/migrations/0004_specification_needs_reference_cascade.mjs
✅ Files skipped from review due to trivial changes (4)
- .markdownlint-cli2.jsonc
- tests/unit/specification-item-statuses-dal.test.ts
- .github/instructions/next-runtime.instructions.md
- tests/integration/requirements-specification-detail.md
…d files - Updated test files to replace instances of "package" with "specification" for clarity and consistency. - Adjusted seed files to reflect the change from packages to specifications, including updates to data structures and comments. - Ensured all related mock implementations and API responses are aligned with the new terminology.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
…nd documentation Co-authored-by: Copilot <copilot@github.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/guide/generate-guide.spec.ts (1)
1222-1233:⚠️ Potential issue | 🟠 MajorFail fast when the underlag panel doesn't load or has no rows.
Line 1225 swallows the visibility timeout, and the
rowCount > 0guard then skips the entire deviation workflow silently. For a guide generation test, this can produce an incomplete guide without failing the test, masking regressions in selectors or test data setup.💡 Suggested fix
- await page - .locator('[data-specification-detail-list-panel="items"]') - .waitFor({ state: 'visible', timeout: 15_000 }) - .catch(() => {}) + await expect( + page.locator('[data-specification-detail-list-panel="items"]'), + ).toBeVisible({ timeout: 15_000 }) // Scope to the left panel (items in specification) — right panel is "available" requirements const allRows = page.locator( '[data-specification-detail-list-panel="items"] tbody tr', ) const rowCount = await allRows.count() + expect( + rowCount, + 'Expected seeded requirements in specification ETJANST-UPP-2026', + ).toBeGreaterThan(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/guide/generate-guide.spec.ts` around lines 1222 - 1233, The test swallows the waitFor timeout and then silently skips the deviation workflow when no rows are present; update the block around page.locator('[data-specification-detail-list-panel="items"]').waitFor(...) and the subsequent allRows/rowCount check so that the waitFor error is not caught (or rethrow the error) and add an explicit assertion that rowCount > 0 (throwing a clear error message referencing the specification panel) instead of simply skipping when rowCount is 0; reference the page.locator call, the allRows locator, and the rowCount variable when making these changes.tests/quality/QUALITY.md (1)
85-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRe-verify the
requirements.tsline anchors in these scenarios.Scenario 2 now points reviewers at
lib/dal/requirements.ts:1107-1129, and Scenario 3 points at1452-1468, but the current archiving/publishing logic in the provided file lives much earlier (initiateArchiving()around Lines 960-966 andtransitionStatus()around Lines 1197-1207). Leaving stale anchors here makes the audit instructions point at the wrong code.As per coding guidelines, "For changes to existing covered invariants: update the matching Fitness Scenario wording, re-verify cited line ranges, and update test case."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/quality/QUALITY.md` around lines 85 - 127, Update the outdated line anchors and tests to point at the current locations of the archiving/publishing logic: locate initiateArchiving() and transitionStatus() in lib/dal/requirements.ts (they now live earlier than the anchors in QUALITY.md) and correct the quoted line ranges in the Scenario 2 and Scenario 3 sections; also re-run and update the functional test selector/test case wording if necessary so the verification commands and test name match the updated code positions and behavior.app/api/specifications/route.ts (1)
15-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject malformed and ambiguous specification payloads before insert.
This handler still 500s on malformed JSON, never validates
name, and accepts digit-onlyuniqueIdvalues even though sibling/specifications/[id]routes treat/^\d+$/as a numeric record id first. That makes a slug like"123"ambiguous or unreachable once created.Suggested hardening
export async function POST(request: NextRequest) { const db = await getRequestSqlServerDataSource() - const body = (await request.json()) as Parameters< - typeof createSpecification - >[1] + let body: unknown + try { + body = await request.json() + } catch { + return NextResponse.json({ error: 'invalid_json' }, { status: 400 }) + } + + if ( + typeof body !== 'object' || + body === null || + typeof (body as { uniqueId?: unknown }).uniqueId !== 'string' || + !(body as { uniqueId: string }).uniqueId.trim() || + typeof (body as { name?: unknown }).name !== 'string' || + !(body as { name: string }).name.trim() || + /^\d+$/.test((body as { uniqueId: string }).uniqueId.trim()) + ) { + return NextResponse.json({ error: 'invalid_request' }, { status: 400 }) + } if ( - !body?.uniqueId || - typeof body.uniqueId !== 'string' || - !body.uniqueId.trim() + await isSlugTaken(db, (body as { uniqueId: string }).uniqueId.trim()) ) { - return NextResponse.json({ error: 'invalid_request' }, { status: 400 }) - } - - if (await isSlugTaken(db, body.uniqueId)) { return NextResponse.json({ error: 'slug_taken' }, { status: 409 }) } - const spec = await createSpecification(db, body) + const spec = await createSpecification( + db, + body as Parameters<typeof createSpecification>[1], + ) return NextResponse.json(spec, { status: 201 }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/specifications/route.ts` around lines 15 - 34, Update the POST handler to validate and reject malformed/ambiguous payloads before DB calls: wrap request.json() in a try/catch to return 400 on invalid JSON, add validation for body.name (e.g., non-empty string) and ensure body.uniqueId is a non-empty trimmed string and not purely digits (reject /^\d+$/ to avoid numeric-slug ambiguity), return appropriate 400 responses for validation failures before calling getRequestSqlServerDataSource/isSlugTaken/createSpecification and only proceed to isSlugTaken/createSpecification when all checks pass.lib/dal/requirements-specifications.ts (1)
844-893:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate all referenced IDs before the write path.
needsReferenceIdis the only foreign key existence-checked here. InvalidrequirementAreaId,requirementCategoryId,requirementTypeId,qualityCharacteristicId,riskLevelId,scenarioIds, ornormReferenceIdscurrently fall through to the later inserts, which turns bad client input into SQL failures/500s for both create and update flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/dal/requirements-specifications.ts` around lines 844 - 893, The code only existence-checks needsReferenceId but lets other foreign keys (requirementAreaId, requirementCategoryId, requirementTypeId, qualityCharacteristicId, riskLevelId, scenarioIds, normReferenceIds) flow to DB and cause 500s; add pre-write validation for each referenced id using the same pattern as needsReferenceId: normalize the id(s) (use normalizeOptionalForeignKeyId and dedupePositiveIntegerIds already in this file), then call the appropriate lookup functions (e.g. getRequirementAreaById, getRequirementCategoryById, getRequirementTypeById, getQualityCharacteristicById, getRiskLevelById, getScenarioById or a batch/get-by-spec helper, and getNormReferenceById) to ensure each id exists and belongs to the relevant specification/tenant, and throw validationError with a clear message if any lookup returns falsy before returning the prepared object.lib/requirements/service.ts (1)
1905-1916:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize the DAL deviation buckets before building
counts.
countDeviationsBySpecification()returns aRecord<string, number>keyed bydecision_kind, but this code uses it as if it already hadtotal,pending,approved, andrejected. That makes the summary text andListDeviationsOutput.countsincorrect whenever the DAL returns raw decision buckets.Suggested fix
- const counts = await countDeviationsBySpecification( - db, - specificationId, - ) + const decisionCounts = await countDeviationsBySpecification( + db, + specificationId, + ) + const approved = + decisionCounts[String(DEVIATION_APPROVED)] ?? 0 + const rejected = + decisionCounts[String(DEVIATION_REJECTED)] ?? 0 + const total = Object.values(decisionCounts).reduce( + (sum, count) => sum + count, + 0, + ) + const counts = { + approved, + pending: total - approved - rejected, + rejected, + total, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/requirements/service.ts` around lines 1905 - 1916, countDeviationsBySpecification returns a Record keyed by decision_kind, but the code treats its result as an object with total/pending/approved/rejected; fix by normalizing the DAL buckets into a proper counts object before building the summary and returning ListDeviationsOutput.counts: after calling countDeviationsBySpecification(db, specificationId) map the returned record into numeric values for pending, approved, rejected (treat missing keys as 0), compute total as either the sum of those three or sum of all buckets from the record, then use that normalized counts object in the summary string and in the output; update usages around resolveSpecificationIdOrThrow, listDeviationsForSpecification, countDeviationsBySpecification and the local counts variable to reference the normalized counts.
🧹 Nitpick comments (2)
scripts/db-sqlserver-admin.mjs (1)
33-36: ⚡ Quick winOnly register actual migration classes.
Right now any exported function is appended to
migrations, so a helper export from a migration module would also be treated as a migration. Narrow this to exports that actually look like TypeORM migration classes before passing them on.Possible tightening
const module = await import(moduleUrl) for (const exported of Object.values(module)) { - if (typeof exported === 'function' && !seen.has(exported)) { + const isMigrationClass = + typeof exported === 'function' && + typeof exported.prototype?.up === 'function' && + typeof exported.prototype?.down === 'function' + if (isMigrationClass && !seen.has(exported)) { seen.add(exported) classes.push(exported) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/db-sqlserver-admin.mjs` around lines 33 - 36, The loop that adds every exported function into classes (variables exported, seen, classes) should be tightened to only collect actual TypeORM migration classes; change the condition so that instead of typeof exported === 'function' you additionally check that exported.prototype exists and exported.prototype.up and exported.prototype.down are functions (i.e., the export looks like a Migration class), and only then add it to seen and classes; this will prevent helper functions from being treated as migrations.lib/mcp/server.ts (1)
1386-1407: ⚡ Quick winPoint clients to the exact fields they should echo into these tools.
These descriptions tell clients to use a specification ID or slug, but they never name the source fields from
requirements_list_specifications. Please spell outstructuredContent.specifications[].id -> specificationIdandstructuredContent.specifications[].uniqueId -> specificationSlugso clients can chain the tools deterministically.As per coding guidelines: "State prerequisite tool calls and exact source/destination fields for values clients must echo in tool descriptions."
Also applies to: 1477-1498, 1563-1578
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mcp/server.ts` around lines 1386 - 1407, Update the tool descriptions in the inputSchema for the requirements_list_specifications tool to explicitly map source fields clients must echo: state that structuredContent.specifications[].id should be passed as specificationId and structuredContent.specifications[].uniqueId should be passed as specificationSlug; update the descriptive text on the specificationId and specificationSlug zod fields to include these source→target mappings so clients can deterministically chain tool calls (also apply the same explicit mapping wording to the other occurrences of the same tool descriptions in the file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot-instructions.md:
- Line 37: Update the sync rule sentence to also require updating
.github/skills/run-spec-audit/references/scrutiny-areas.md whenever you add,
rename, or remove outward-facing lifecycle/specification-item status/MCP
tool/report column/admin-default behavior entries; change the instruction that
references tests/quality/QUALITY.md and tests/quality/functional.test.ts to
explicitly include scrutiny-areas.md and mention the three-file fitness-scenario
contract (QUALITY.md heading, functional.test.ts test case, and
scrutiny-areas.md entry) must be changed together.
In
`@app/`[locale]/specifications/[slug]/requirements-specification-detail-client.tsx:
- Around line 331-355: fetchSpecificationMeta currently swallows non-OK
responses and fetchSpecificationItems returns false but the initial Promise.all
path ignores that, so 500s become treated as "not found"/empty; modify
fetchSpecificationMeta to accept an optional { throwOnError?: boolean } param
(like fetchSpecificationItems) and when res.ok is false either throw new
Error('Failed to refresh specification meta') if throwOnError is true or return
false/undefined so callers know it failed; update the initial loading logic (the
Promise.all that calls fetchSpecificationMeta and fetchSpecificationItems) to
call both with throwOnError: true (or check their boolean return values) and
handle failures by setting a distinct load error/state instead of setting
specificationNotFound or noItems; also update other call sites (the ones around
lines 435-439) to follow the same pattern and not silently treat false as a
successful empty state.
- Around line 321-324: The Set `specificationItemIds` is built from mixed
namespaces (library rows use requirement.id while local rows use
specification_local_requirement.id), causing unrelated catalog requirements to
be excluded; change the construction to only include IDs from library-linked
rows by filtering specificationItems for rows with a `requirement` and mapping
to `requirement.id` (e.g., specificationItemIds = new
Set(specificationItems.filter(r => r.requirement).map(r => r.requirement.id))),
and apply the same filter/map fix to the other occurrence that computes the ID
set used to filter `rightRows`.
In `@app/api/specifications/`[id]/items/[itemId]/route.ts:
- Around line 110-126: The current PATCH does a read with
getSpecificationItemByRef then calls updateSpecificationItemFieldsByItemRef
which repeats the lookup, creating a race where the item can be deleted between
calls and produce a 500 HTML response; remove the initial
getSpecificationItemByRef call and instead call
updateSpecificationItemFieldsByItemRef(db, specificationId, decodedItemRef,
body) directly, then inspect its return value (e.g., affected rows, updated
record, or null) to determine if the item existed—if the update helper indicates
no record was found, return NextResponse.json({ error: 'Item not found in
specification' }, { status: 404 }); otherwise proceed normally. Ensure
updateSpecificationItemFieldsByItemRef consistently returns a falsy value when
no row was updated so this route can reliably map that to a JSON 404.
In `@app/api/specifications/`[id]/items/route.ts:
- Around line 315-320: The catch block currently exposes error.message to
clients; instead, log the actual error server-side (e.g., console.error(error)
or the existing logger if available) and always return a stable JSON message
(e.g., "Failed to unlink requirements") with status 500. Specifically, update
the catch handling that defines const message and calls NextResponse.json so it
does not include error.message, ensure the raw error is logged (use the error
variable) and keep the client-facing response deterministic via
NextResponse.json({ error: 'Failed to unlink requirements' }, { status: 500 }).
In `@devfile.yaml`:
- Around line 161-170: The devfile currently commits a known admin password
(KEYCLOAK_ADMIN_PASSWORD = admin) and exposes Keycloak publicly (endpoints name:
keycloak), creating takeover risk; remove the hardcoded value for
KEYCLOAK_ADMIN_PASSWORD and instead reference an injected secret (e.g., use an
env valueFrom/secretRef like keycloak-admin-secret) and ensure the secret is not
checked into source control, and change the keycloak endpoint exposure from
public to internal (or remove the public exposure) so Keycloak is not publicly
reachable by default while local/dev realms are imported via
KC_IMPORT_REALM_DIR.
In `@docs/database-schema.md`:
- Around line 1155-1161: Update the `unused_1` entry in the
`requirements_specification_items` table documentation to explicitly mark it as
a legacy/deprecated naming exception: state that the column is intentionally
retained under the non-standard name for migration compatibility, is deprecated
and should not be used in new code, and include a short rationale (legacy
placeholder kept for backwards compatibility) alongside the existing “Retired”
text so readers know this violates the normal naming standard intentionally.
In `@docs/requirements-ui-behaviour.md`:
- Around line 298-300: Replace the outdated left-panel label "Krav i
kravunderlag" with the current label "Krav i underlaget" in the sentence
describing library requirement inline detail metadata so the phrase matches
other updated specification-detail docs; update the occurrence that reads "When
a library requirement is opened from the specification list `Krav i
kravunderlag`" to use `Krav i underlaget` (search for that exact phrase to
locate the change).
In `@lib/mcp/server.ts`:
- Around line 1319-1328: The inputSchema.describe for nameSearch overstates
behavior by claiming it filters both Swedish and English names while the handler
(requirements_list_specifications) currently only matches p.name; update the
description text on inputSchema.nameSearch to explicitly state it's a
case-insensitive substring filter applied to the primary name field (p.name)
only, or alternatively implement the intended bilingual matching in the
requirements_list_specifications query (adding p.name_sv to the filter);
reference inputSchema.nameSearch and the requirements_list_specifications
handler / p.name filter when making the change.
In `@lib/slug.ts`:
- Around line 38-39: The JSDoc example in lib/slug.ts showing input
"Säkerhetslyft Q2" → "SAKLYFT-INFOR-Q2" is incorrect; run the actual slug
function (the JSDoc block above the slug generation implementation) with that
input to observe the real output and replace the example output string to match
the function's behavior. Update only the comment example (the input/output pair)
in the JSDoc above the slug function so it reflects the actual returned slug for
"Säkerhetslyft Q2".
In `@scripts/db-sqlserver-admin.mjs`:
- Around line 11-20: listMigrationFilenames currently only accepts ".mjs" which
diverges from the runtime DataSource glob (which uses "*.{js,ts}"); update
listMigrationFilenames to use the same extension matcher as the runtime (e.g.,
accept .js and .ts or reuse the runtime's extension list/constant) by changing
the filter in listMigrationFilenames (and referencing MIGRATIONS_DIR and the
function name) so both admin script and lib/typeorm/sqlserver-config.ts share
the same allowed extensions/pattern.
In `@tests/guide/generate-guide.spec.ts`:
- Around line 1296-1299: The current page.waitForResponse call that sets
deviationResPromise uses only a URL matcher and can match the initial GET;
update the wait to target the POST submit response by adding a predicate that
checks both the URL (/\/api\/specification-item-deviations\//) and that
request.method() === 'POST' (i.e., change the page.waitForResponse usage where
deviationResPromise is defined to only resolve for POST requests), so the
promise only resolves on the submit POST rather than the earlier GET.
In `@tests/integration/specifications-list.md`:
- Around line 48-52: Reword the repeated step sentences that start with "Assert"
in the specifications list so they don't all begin the same way; for example
change one or two of the lines like "Assert the browser is on
`/sv/specifications`" or "Assert the page title contains 'Kravunderlag'" to
"Verify the browser is on `/sv/specifications`" or "Check that the page title
contains 'Kravunderlag'" to satisfy the style rule while keeping the same
assertions and wording otherwise.
---
Outside diff comments:
In `@app/api/specifications/route.ts`:
- Around line 15-34: Update the POST handler to validate and reject
malformed/ambiguous payloads before DB calls: wrap request.json() in a try/catch
to return 400 on invalid JSON, add validation for body.name (e.g., non-empty
string) and ensure body.uniqueId is a non-empty trimmed string and not purely
digits (reject /^\d+$/ to avoid numeric-slug ambiguity), return appropriate 400
responses for validation failures before calling
getRequestSqlServerDataSource/isSlugTaken/createSpecification and only proceed
to isSlugTaken/createSpecification when all checks pass.
In `@lib/dal/requirements-specifications.ts`:
- Around line 844-893: The code only existence-checks needsReferenceId but lets
other foreign keys (requirementAreaId, requirementCategoryId, requirementTypeId,
qualityCharacteristicId, riskLevelId, scenarioIds, normReferenceIds) flow to DB
and cause 500s; add pre-write validation for each referenced id using the same
pattern as needsReferenceId: normalize the id(s) (use
normalizeOptionalForeignKeyId and dedupePositiveIntegerIds already in this
file), then call the appropriate lookup functions (e.g. getRequirementAreaById,
getRequirementCategoryById, getRequirementTypeById,
getQualityCharacteristicById, getRiskLevelById, getScenarioById or a
batch/get-by-spec helper, and getNormReferenceById) to ensure each id exists and
belongs to the relevant specification/tenant, and throw validationError with a
clear message if any lookup returns falsy before returning the prepared object.
In `@lib/requirements/service.ts`:
- Around line 1905-1916: countDeviationsBySpecification returns a Record keyed
by decision_kind, but the code treats its result as an object with
total/pending/approved/rejected; fix by normalizing the DAL buckets into a
proper counts object before building the summary and returning
ListDeviationsOutput.counts: after calling countDeviationsBySpecification(db,
specificationId) map the returned record into numeric values for pending,
approved, rejected (treat missing keys as 0), compute total as either the sum of
those three or sum of all buckets from the record, then use that normalized
counts object in the summary string and in the output; update usages around
resolveSpecificationIdOrThrow, listDeviationsForSpecification,
countDeviationsBySpecification and the local counts variable to reference the
normalized counts.
In `@tests/guide/generate-guide.spec.ts`:
- Around line 1222-1233: The test swallows the waitFor timeout and then silently
skips the deviation workflow when no rows are present; update the block around
page.locator('[data-specification-detail-list-panel="items"]').waitFor(...) and
the subsequent allRows/rowCount check so that the waitFor error is not caught
(or rethrow the error) and add an explicit assertion that rowCount > 0 (throwing
a clear error message referencing the specification panel) instead of simply
skipping when rowCount is 0; reference the page.locator call, the allRows
locator, and the rowCount variable when making these changes.
In `@tests/quality/QUALITY.md`:
- Around line 85-127: Update the outdated line anchors and tests to point at the
current locations of the archiving/publishing logic: locate initiateArchiving()
and transitionStatus() in lib/dal/requirements.ts (they now live earlier than
the anchors in QUALITY.md) and correct the quoted line ranges in the Scenario 2
and Scenario 3 sections; also re-run and update the functional test
selector/test case wording if necessary so the verification commands and test
name match the updated code positions and behavior.
---
Nitpick comments:
In `@lib/mcp/server.ts`:
- Around line 1386-1407: Update the tool descriptions in the inputSchema for the
requirements_list_specifications tool to explicitly map source fields clients
must echo: state that structuredContent.specifications[].id should be passed as
specificationId and structuredContent.specifications[].uniqueId should be passed
as specificationSlug; update the descriptive text on the specificationId and
specificationSlug zod fields to include these source→target mappings so clients
can deterministically chain tool calls (also apply the same explicit mapping
wording to the other occurrences of the same tool descriptions in the file).
In `@scripts/db-sqlserver-admin.mjs`:
- Around line 33-36: The loop that adds every exported function into classes
(variables exported, seen, classes) should be tightened to only collect actual
TypeORM migration classes; change the condition so that instead of typeof
exported === 'function' you additionally check that exported.prototype exists
and exported.prototype.up and exported.prototype.down are functions (i.e., the
export looks like a Migration class), and only then add it to seen and classes;
this will prevent helper functions from being treated as migrations.
🪄 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: 8876d443-41ed-4b49-a049-049ff7cd9a94
⛔ Files ignored due to path filters (1)
docs/guide/images/029-krav-i-kravunderlag-expanderat.pngis excluded by!**/*.png
📒 Files selected for processing (104)
.github/copilot-instructions.md.github/instructions/database-schema.instructions.md.github/instructions/detail-pane-layout.instructions.md.github/instructions/quality-spec.instructions.md.github/skills/run-spec-audit/references/integration-contracts.md.github/skills/run-spec-audit/references/scrutiny-areas.md.github/skills/update-guide-spec/SKILL.mdREADME.mdapp/[locale]/requirements/[id]/_detail/AddToSpecificationDialog.tsxapp/[locale]/requirements/[id]/_detail/RequirementActionRail.tsxapp/[locale]/requirements/[id]/_detail/RequirementReportMenu.tsxapp/[locale]/requirements/[id]/_detail/types.tsapp/[locale]/requirements/[id]/_detail/use-add-to-specification-dialog.tsapp/[locale]/requirements/[id]/_detail/use-deviation-workflow.tsapp/[locale]/requirements/[id]/_detail/use-specification-item-context.tsapp/[locale]/requirements/[id]/requirement-detail-client.tsxapp/[locale]/specifications/[slug]/page.tsxapp/[locale]/specifications/[slug]/reports/print/list/page.tsxapp/[locale]/specifications/[slug]/requirements-specification-detail-client.tsxapp/[locale]/specifications/[slug]/specification-edit-panel.tsxapp/[locale]/specifications/specifications-client.tsxapp/api/catalog/specification-item-statuses/[id]/route.tsapp/api/catalog/specification-item-statuses/route.tsapp/api/specification-item-deviations/[itemId]/route.tsapp/api/specifications/[id]/deviations/route.tsapp/api/specifications/[id]/items/[itemId]/route.tsapp/api/specifications/[id]/items/route.tsapp/api/specifications/[id]/local-requirements/[localRequirementId]/route.tsapp/api/specifications/[id]/local-requirements/route.tsapp/api/specifications/[id]/needs-references/route.tsapp/api/specifications/[id]/report-items/route.tsapp/api/specifications/[id]/route.tsapp/api/specifications/route.tscomponents/Navigation.tsxcomponents/SpecificationLocalRequirementDetailClient.tsxcspell.jsoncdev/keycloak/realm-kravhantering-dev.jsondevfile.yamldocs/arkitekturbeskrivning-kravhantering.mddocs/auth-developer-workflow.mddocs/auth-how-it-works.mddocs/database-schema.mddocs/developer-mode-overlay.mddocs/dogfood-seed.mddocs/guide/README.mddocs/lifecycle-workflow.mddocs/mcp-server-contributor-guide.mddocs/mcp-server-user-guide.mddocs/openshift-devspaces.mddocs/requirements-ui-behaviour.mddocs/security-ci.mddocs/sql-server-developer-workflow.mdlib/dal/deviations.tslib/dal/requirements-specifications.tslib/dal/requirements.tslib/dal/specification-item-statuses.tslib/mcp/server.tslib/reports/data/fetch-deviation.tslib/reports/data/fetch-specification-items.tslib/reports/templates/deviation-review-template.tslib/requirements/auth.tslib/requirements/service.tslib/slug.tsmessages/en.jsonmessages/sv.jsonscripts/__tests__/db-sqlserver-admin.test.mjsscripts/db-sqlserver-admin.mjstests/guide/generate-guide.spec.tstests/integration/developer-mode-overlay.mdtests/integration/developer-mode-overlay.spec.tstests/integration/requirements-specification-detail.mdtests/integration/requirements-specification-detail.spec.tstests/integration/specifications-list.mdtests/integration/specifications-list.spec.tstests/quality/QUALITY.mdtests/quality/functional.test.tstests/support/oidc-mock.tstests/unit/deviations-dal.test.tstests/unit/dogfood-seed.test.tstests/unit/mcp-http.test.tstests/unit/reference-data-developer-mode.test.tsxtests/unit/report-error-details.test.tstests/unit/requirement-detail-client.test.tsxtests/unit/requirement-detail-hooks.test.tsxtests/unit/requirement-report-menu.test.tsxtests/unit/requirements-dal.test.tstests/unit/requirements-service.test.tstests/unit/requirements-specification-detail-client.test.tsxtests/unit/requirements-specification-item-route.test.tstests/unit/requirements-specification-items-route.test.tstests/unit/requirements-specifications-dal.test.tstests/unit/requirements-table.test.tsxtests/unit/specification-edit-panel.test.tsxtests/unit/specification-item-statuses-client.test.tsxtests/unit/specification-item-statuses-dal.test.tstests/unit/specification-local-requirement-detail-client.test.tsxtests/unit/specification-local-requirement-route.test.tstests/unit/specification-report-pages.test.tsxtests/unit/specifications-client.test.tsxtests/unit/specifications-page.test.tsxtests/unit/taxonomy-routes.test.tstypeorm/seed-dogfood-build.mjstypeorm/seed-dogfood.mjstypeorm/seed.mjs
✅ Files skipped from review due to trivial changes (19)
- .github/instructions/quality-spec.instructions.md
- docs/auth-how-it-works.md
- docs/sql-server-developer-workflow.md
- .github/instructions/detail-pane-layout.instructions.md
- .github/instructions/database-schema.instructions.md
- docs/security-ci.md
- README.md
- tests/unit/report-error-details.test.ts
- docs/openshift-devspaces.md
- docs/mcp-server-user-guide.md
- docs/auth-developer-workflow.md
- app/api/specifications/[id]/route.ts
- docs/developer-mode-overlay.md
- lib/reports/templates/deviation-review-template.ts
- tests/integration/requirements-specification-detail.spec.ts
- .github/skills/update-guide-spec/SKILL.md
- tests/integration/developer-mode-overlay.md
- docs/guide/README.md
- tests/unit/dogfood-seed.test.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- app/api/specifications/[id]/needs-references/route.ts
- tests/unit/mcp-http.test.ts
- app/api/catalog/specification-item-statuses/[id]/route.ts
- docs/mcp-server-contributor-guide.md
- app/[locale]/requirements/[id]/_detail/types.ts
- tests/integration/developer-mode-overlay.spec.ts
- lib/dal/specification-item-statuses.ts
- app/api/specifications/[id]/deviations/route.ts
- app/[locale]/requirements/[id]/_detail/use-deviation-workflow.ts
- app/[locale]/requirements/[id]/_detail/AddToSpecificationDialog.tsx
- app/api/specifications/[id]/local-requirements/[localRequirementId]/route.ts
- app/[locale]/requirements/[id]/_detail/RequirementReportMenu.tsx
- tests/integration/specifications-list.spec.ts
- cspell.jsonc
- app/[locale]/requirements/[id]/_detail/use-add-to-specification-dialog.ts
- app/[locale]/requirements/[id]/_detail/use-specification-item-context.ts
- docs/lifecycle-workflow.md
- components/SpecificationLocalRequirementDetailClient.tsx
- docs/arkitekturbeskrivning-kravhantering.md
- .github/skills/run-spec-audit/references/scrutiny-areas.md
- tests/quality/functional.test.ts
- lib/dal/deviations.ts
4 tasks
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/quality/QUALITY.md (1)
58-385:⚠️ Potential issue | 🔴 CriticalSplit Scenario 12 into three matching test names across all three files.
Scenario 12inQUALITY.mdandscrutiny-areas.mdhas been split into three separate tests infunctional.test.ts(12a,12b,12c), butQUALITY.mdstill contains a single unifiedScenario 12heading. The three-file fitness scenario sibling contract requires the test name infunctional.test.tsto match verbatim the heading inQUALITY.mdand the entry inscrutiny-areas.md.Either split
Scenario 12inQUALITY.mdinto three separate scenario sections (Scenario 12a,12b,12c) with matching updates toscrutiny-areas.md, or consolidate the three test cases infunctional.test.tsback into a singleit('Scenario 12: ...')test. Keep all three components in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/quality/QUALITY.md` around lines 58 - 385, The markdown heading "Scenario 12" in QUALITY.md no longer matches the three test names added in functional.test.ts ("Scenario 12a", "Scenario 12b", "Scenario 12c") (and the sibling entry in scrutiny-areas.md), causing the three-file contract to break; either split the single "Scenario 12" heading/content in QUALITY.md into three verbatim headings "Scenario 12a", "Scenario 12b", "Scenario 12c" and make the identical changes in scrutiny-areas.md, or revert the three tests in functional.test.ts back into one single test named exactly "Scenario 12: concurrent archiving attempts are atomic and strictly targeted"; ensure the scenario headings in QUALITY.md/scrutiny-areas.md and test names in functional.test.ts match verbatim.lib/mcp/server.ts (2)
1386-1407: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMake these tool descriptions name the exact fields clients must echo.
The renamed tool contracts still stop at “use
specificationId/specificationSlug” or “passrequirementIds”. Please spell out the copy path in the description, e.g.requirements_list_specifications.specifications[].id -> specificationId,requirements_list_specifications.specifications[].uniqueId -> specificationSlug, and the relevant[].id -> requirementIdssource for add/remove. That keeps the MCP surface self-describing for clients. As per coding guidelines: "State prerequisite tool calls and exact source/destination fields for values clients must echo in tool descriptions" and "Do not rely on user docs alone to teach MCP clients how to call a tool; teach via tool contract definition."📝 Proposed description updates
- 'List requirements (krav) linked to a specific requirements specification, with optional description search. Identify the specification with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2") from requirements_list_specifications.', + 'List requirements (krav) linked to a specific requirements specification, with optional description search. First call requirements_list_specifications, then copy specifications[].id to specificationId or specifications[].uniqueId to specificationSlug.', - 'Link one or more requirements to a requirements specification. Requirements must have a published version; those without are skipped and returned in skippedIds. Optionally attach a needs reference text to all added items. Identify the specification with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2").', + 'Link one or more requirements to a requirements specification. First call requirements_list_specifications and copy specifications[].id to specificationId or specifications[].uniqueId to specificationSlug. Populate requirementIds from requirements_query_catalog.items[].id or requirements_get_requirement.requirement.id. Requirements must have a published version; those without are skipped and returned in skippedIds. Optionally attach a needs reference text to all added items.', - 'Unlink one or more requirements from a requirements specification. The requirements themselves are not deleted. Identify the specification with specificationId (numeric) or specificationSlug (e.g. "SAKLYFT-INFOR-Q2").', + 'Unlink one or more requirements from a requirements specification. First call requirements_list_specifications and copy specifications[].id to specificationId or specifications[].uniqueId to specificationSlug. Populate requirementIds from requirements_get_specification_items.items[].id. The requirements themselves are not deleted.',Also applies to: 1477-1504, 1563-1584
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mcp/server.ts` around lines 1386 - 1407, Update the tool description strings so they explicitly tell clients which exact fields to copy from prior tool outputs into these inputs: e.g. for the requirements_list_specifications tool, state that clients must echo requirements_list_specifications.specifications[].id -> specificationId and requirements_list_specifications.specifications[].uniqueId -> specificationSlug; for add/remove requirement tools, state that requirements_list_specifications.specifications[].requirements[].id -> requirementIds (or the exact array path used). Edit the human-readable description in the tool contract where inputSchema and fields specificationId, specificationSlug, and requirementIds are declared so the copy paths are verbatim in the text (refer to inputSchema, specificationId, specificationSlug, and requirementIds and update the top-level tool description strings and any similar descriptions elsewhere mentioned).
1310-1631:⚠️ Potential issue | 🟠 MajorAdd a Fitness Scenario for the new specification MCP tools' behavior and contracts.
These four tools (
requirements_list_specifications,requirements_get_specification_items,requirements_add_to_specification,requirements_remove_from_specification) are new outward-facing MCP operations. Per the coding guidelines, new MCP tool additions require a Fitness Scenario intests/quality/QUALITY.mdwith a matching test intests/quality/functional.test.tsand scrutiny entry in.github/skills/run-spec-audit/references/scrutiny-areas.md.Scenario 10 (MCP tool inventory) only verifies that the tool count matches documentation; it does not test the tools' actual behavior, schema validation, field contracts, or error handling. The specification tools need a dedicated behavior scenario covering their input/output schemas, identifier validation (e.g., exactly-one-of-two logic for
specificationId/specificationSlug), and correct data flow through service layer to database DAL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mcp/server.ts` around lines 1310 - 1631, Add a Fitness Scenario and tests for the four new MCP tools to validate their behavior and contracts: create a scenario entry in tests/quality/QUALITY.md that describes expected input/output schemas and behaviors for requirements_list_specifications, requirements_get_specification_items, requirements_add_to_specification, and requirements_remove_from_specification; add corresponding tests in tests/quality/functional.test.ts that exercise schema validation (including the exactly-one-of-two superRefine rule for specificationId vs specificationSlug), responseFormat/locale handling, happy-path data flow through service methods listSpecifications, getSpecificationItems, addToSpecification, removeFromSpecification, and error cases (validation failures and service errors); and add a scrutiny note in .github/skills/run-spec-audit/references/scrutiny-areas.md referencing the new scenario and listing the specific contract checks (schemas, identifier validation, skippedIds behavior for addToSpecification, and removedCount/addedCount semantics).
🤖 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/api/specifications/`[id]/items/[itemId]/route.ts:
- Line 110: Guard the decodeURIComponent call for itemId to avoid uncaught
URIError: wrap the call to decodeURIComponent(itemId) inside a try/catch in the
route handler (where decodedItemRef is assigned), and on failure return a 400
JSON response (e.g., { error: "Invalid itemId" }) instead of throwing; ensure
the catch only handles URIError/decoding failures and preserves normal flow for
valid itemId values.
In `@tests/quality/QUALITY.md`:
- Around line 189-204: Update the documentation in tests/quality/QUALITY.md to
use the correct deviated status id: replace the hardcoded
"specificationItemStatusId = 5" with the implemented value
(DEVIATED_SPECIFICATION_ITEM_STATUS_ID, which is 2) or explicitly state
"specificationItemStatusId = 2"; ensure the scenario text that references the
guards in updateSpecificationItemFields() and
updateSpecificationLocalRequirementFields() reflects the actual constant used so
the narrative matches the implementation.
---
Outside diff comments:
In `@lib/mcp/server.ts`:
- Around line 1386-1407: Update the tool description strings so they explicitly
tell clients which exact fields to copy from prior tool outputs into these
inputs: e.g. for the requirements_list_specifications tool, state that clients
must echo requirements_list_specifications.specifications[].id ->
specificationId and requirements_list_specifications.specifications[].uniqueId
-> specificationSlug; for add/remove requirement tools, state that
requirements_list_specifications.specifications[].requirements[].id ->
requirementIds (or the exact array path used). Edit the human-readable
description in the tool contract where inputSchema and fields specificationId,
specificationSlug, and requirementIds are declared so the copy paths are
verbatim in the text (refer to inputSchema, specificationId, specificationSlug,
and requirementIds and update the top-level tool description strings and any
similar descriptions elsewhere mentioned).
- Around line 1310-1631: Add a Fitness Scenario and tests for the four new MCP
tools to validate their behavior and contracts: create a scenario entry in
tests/quality/QUALITY.md that describes expected input/output schemas and
behaviors for requirements_list_specifications,
requirements_get_specification_items, requirements_add_to_specification, and
requirements_remove_from_specification; add corresponding tests in
tests/quality/functional.test.ts that exercise schema validation (including the
exactly-one-of-two superRefine rule for specificationId vs specificationSlug),
responseFormat/locale handling, happy-path data flow through service methods
listSpecifications, getSpecificationItems, addToSpecification,
removeFromSpecification, and error cases (validation failures and service
errors); and add a scrutiny note in
.github/skills/run-spec-audit/references/scrutiny-areas.md referencing the new
scenario and listing the specific contract checks (schemas, identifier
validation, skippedIds behavior for addToSpecification, and
removedCount/addedCount semantics).
In `@tests/quality/QUALITY.md`:
- Around line 58-385: The markdown heading "Scenario 12" in QUALITY.md no longer
matches the three test names added in functional.test.ts ("Scenario 12a",
"Scenario 12b", "Scenario 12c") (and the sibling entry in scrutiny-areas.md),
causing the three-file contract to break; either split the single "Scenario 12"
heading/content in QUALITY.md into three verbatim headings "Scenario 12a",
"Scenario 12b", "Scenario 12c" and make the identical changes in
scrutiny-areas.md, or revert the three tests in functional.test.ts back into one
single test named exactly "Scenario 12: concurrent archiving attempts are atomic
and strictly targeted"; ensure the scenario headings in
QUALITY.md/scrutiny-areas.md and test names in functional.test.ts match
verbatim.
🪄 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: fda04c25-ae26-431f-b54a-c58384bece4c
📒 Files selected for processing (9)
.github/copilot-instructions.mdapp/api/specifications/[id]/items/[itemId]/route.tsapp/api/specifications/[id]/items/route.tsdocs/requirements-ui-behaviour.mdlib/mcp/server.tslib/slug.tstests/guide/generate-guide.spec.tstests/quality/QUALITY.mdtests/unit/requirements-specification-items-route.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/guide/generate-guide.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/api/specifications/[id]/items/route.ts
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
This changes all files but the integration tests, skills and instructions, they have been hold back on purpuse to limit the changeset of # of files.
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is