feat: implement atomic archiving operations with transaction - #139
Merged
Conversation
…ansaction handling
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds UI support for an "Archiving Review" label when a version's Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/UI
participant Service as Service Layer
participant DAL as Archiving DAL
participant DB as Database
rect rgba(0,128,0,0.5)
Client->>Service: User triggers initiateArchiving(requirementId, versionId)
end
rect rgba(0,0,128,0.5)
Service->>DAL: initiateArchiving(requirementId, versionId)
DAL->>DB: BEGIN TRANSACTION (SERIALIZABLE)
DAL->>DB: SELECT published version WITH (UPDLOCK,HOLDLOCK)
DB-->>DAL: published version row
DAL->>DB: CHECK for newer draft/review WITH (UPDLOCK,HOLDLOCK)
DB-->>DAL: no blockers
DAL->>DB: UPDATE version SET archive_initiated_at = NOW() OUTPUT INSERTED.id WHERE archive_initiated_at IS NULL
DB-->>DAL: updated id OR 0 rows
DAL->>DB: COMMIT
DB-->>DAL: commit result
DAL-->>Service: success / conflict
Service-->>Client: response (ok | conflict)
end
sequenceDiagram
participant Admin as Admin/UI
participant Service as Service Layer
participant DAL as Archiving DAL
participant DB as Database
Admin->>Service: approveArchiving(requirementId, versionId)
Service->>DAL: approveArchiving(...)
DAL->>DB: BEGIN TRANSACTION (SERIALIZABLE)
DAL->>DB: SELECT version WITH (UPDLOCK,HOLDLOCK) WHERE archive_initiated_at IS NOT NULL AND status = REVIEW
DB-->>DAL: targeted row or none
alt targeted row found
DAL->>DB: UPDATE version SET status = ARCHIVED OUTPUT INSERTED.id WHERE ... AND archive_initiated_at IS NOT NULL
DAL->>DB: UPDATE requirements SET is_archived = 1 WHERE requirement_id = ...
else none
DAL-->>Service: conflict
end
DAL->>DB: COMMIT
DAL-->>Service: success / conflict
Service-->>Admin: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 42 minutes.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
+ Coverage 57.43% 57.66% +0.22%
==========================================
Files 288 289 +1
Lines 17455 17472 +17
Branches 6670 6562 -108
==========================================
+ Hits 10026 10075 +49
+ Misses 7317 7287 -30
+ Partials 112 110 -2
🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/unit/requirements-table.test.tsx (1)
635-671: ⚡ Quick winAdd one English-locale assertion for the new label path.
Since this PR also adds the English copy (
Archiving Review), include alocale="en"case to catch locale-specific regressions in status-label rendering.As per coding guidelines: "When changing visible UI elements, labels, roles, or layout surfaces, update the relevant unit and integration tests."Proposed test addition
+ it('renders the archiving review label in English when archiveInitiatedAt is set', () => { + const rows = [ + makeRow({ + version: { + ...makeRow().version, + status: 2, + statusColor: '#f59e0b', + statusNameEn: 'Review', + statusNameSv: 'Granskning', + archiveInitiatedAt: '2026-04-01T12:00:00.000Z', + }, + }), + ] + render(<RequirementsTable locale="en" rows={rows} />) + + expect(screen.getByText('Archiving Review')).toBeTruthy() + expect(screen.queryByText('Review')).toBeNull() + })🤖 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 635 - 671, Add an English-locale assertion for the new "Archiving Review" label in the RequirementsTable tests: in tests/unit/requirements-table.test.tsx add a case similar to the existing Swedish tests but render <RequirementsTable locale="en" rows={rowsWithArchiveInitiatedAt}> and assert screen.getByText('Archiving Review') is present and screen.queryByText('Review') is null; also add the converse case where archiveInitiatedAt is null to assert 'Review' is present and 'Archiving Review' is null, referencing the existing test helper makeRow and the RequirementsTable component to locate where to add the new assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/lifecycle-workflow.md`:
- Around line 98-101: Update the sentence to say that the status badge reflects
the per-version "resolved" status derived from the effective status logic, not
the raw DB column; explicitly mention that the requirements list label is
computed by EFFECTIVE_STATUS_SQL in lib/dal/requirements.ts (which consolidates
requirement_versions.requirement_status_id and
requirement_versions.archive_initiated_at) so readers know the list view uses
the effective/resolved status rather than the raw
requirement_versions.requirement_status_id column.
In `@lib/dal/requirement-packages.ts`:
- Line 1552: The code is hardcoding archiveInitiatedAt: null for library package
items which hides archive-review status; instead read the linked requirement
version's archive_initiated_at and assign it to archiveInitiatedAt
(converting/normalizing to the same casing/format used elsewhere) when building
the package item (replace the literal null in the package construction logic
that sets archiveInitiatedAt with the linked requirement version's
archive_initiated_at value, falling back to null if absent).
In `@lib/dal/requirements.ts`:
- Around line 948-966: The SELECTs using `TOP (1)` (e.g., the query populating
publishedRows via tx.query against requirement_versions) can hide multiple
matching rows; update the logic to detect ambiguous targets by running a COUNT
or selecting all matching ids (instead of TOP (1)) and if more than one row is
returned throw a conflictError('Ambiguous archive target') before proceeding;
apply the same change to the mirrored approve/cancel queries that rely on TOP
(1), or alternatively enforce and validate a unique filtered index on
requirement_versions (unique on requirement_id + requirement_status_id +
archive_initiated_at filter) and add a runtime check that ensures exactly one
row is returned when querying `requirement_versions` in functions handling
archive/approve/cancel.
In `@tests/quality/functional.test.ts`:
- Around line 848-851: The current assertions on the filtered "fulfilled"
results (variable fulfilled derived from results.filter(r => r.status ===
'fulfilled')) allow two successes which contradicts the test contract; change
the assertions to require exactly one success by replacing the two checks with a
single strict check like expect(fulfilled.length).toBe(1) so the test fails if
both operations succeed.
- Around line 769-799: The two test case titles for Scenario 12 must be
verbatim-equal to the Scenario 12 heading in QUALITY.md; update the it(...)
description strings that wrap the concurrent initiateArchiving (the test calling
initiateArchiving twice) and the concurrent approveArchiving (the test starting
at the next it) so their title text exactly matches the Scenario 12 heading in
QUALITY.md (including punctuation and spacing) to satisfy the repo’s
scenario-name contract used by vitest -t; leave the test bodies (calls to
initiateArchiving, approveArchiving, assertions against getVersionHistory,
STATUS_REVIEW, etc.) unchanged.
In `@tests/quality/QUALITY.md`:
- Around line 352-383: Add a new scrutiny area entry for "Scenario 12" to
.github/skills/run-spec-audit/references/scrutiny-areas.md so the three-file
sync is complete: include the same requirement tag string `[Req: formal —
docs/lifecycle-workflow.md "Two-Step Archiving"]` and the same verify command
shown in QUALITY.md (`npm exec -- vitest run tests/quality/functional.test.ts -t
"Scenario 12"`) and ensure the entry's title/description matches the Scenario 12
wording used in QUALITY.md and the test name in
tests/quality/functional.test.ts.
---
Nitpick comments:
In `@tests/unit/requirements-table.test.tsx`:
- Around line 635-671: Add an English-locale assertion for the new "Archiving
Review" label in the RequirementsTable tests: in
tests/unit/requirements-table.test.tsx add a case similar to the existing
Swedish tests but render <RequirementsTable locale="en"
rows={rowsWithArchiveInitiatedAt}> and assert screen.getByText('Archiving
Review') is present and screen.queryByText('Review') is null; also add the
converse case where archiveInitiatedAt is null to assert 'Review' is present and
'Archiving Review' is null, referencing the existing test helper makeRow and the
RequirementsTable component to locate where to add the new assertions.
🪄 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: c650f543-bc3a-4fcf-b2ac-59d54a03454d
📒 Files selected for processing (19)
app/[locale]/requirements/[id]/requirement-detail-client.tsxcomponents/RequirementsTable.tsxcomponents/StatusStepper.tsxcomponents/VersionHistory.tsxdocs/database-schema.mddocs/lifecycle-workflow.mdlib/dal/requirement-packages.tslib/dal/requirements.tslib/requirements/list-view.tslib/requirements/service.tslib/requirements/status-label.tsmessages/en.jsonmessages/sv.jsontests/quality/QUALITY.mdtests/quality/functional.test.tstests/unit/requirements-dal.test.tstests/unit/requirements-table.test.tsxtests/unit/status-label.test.tstests/unit/status-stepper.test.tsx
…workflow documentation
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/skills/run-spec-audit/references/scrutiny-areas.md:
- Around line 164-179: The markdown has duplicate "## 14" for "Reference Data
Behavioral Contracts" and mis-numbered "AI Generation Contracts"; rename the
heading "Reference Data Behavioral Contracts" from "## 14" to "## 15" and change
"AI Generation Contracts" from "## 15" to "## 16" (and scan the rest of the file
to increment any subsequent section numbers to maintain sequential ordering) so
section numbering is unique and sequential.
🪄 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: 05f5f060-06d9-44b3-a7aa-bbe41412c7b4
📒 Files selected for processing (3)
.github/skills/run-spec-audit/references/scrutiny-areas.mddocs/lifecycle-workflow.mdtests/quality/functional.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/lifecycle-workflow.md
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is