feat(projects): doc-review stamp store + routes (#1802 slice 3)#1834
feat(projects): doc-review stamp store + routes (#1802 slice 3)#1834jaylfc wants to merge 2 commits into
Conversation
Add per-document review_state machine (awaiting_review/approved/changes_requested) with actor recording and timestamps, project-scoped agent-token gated routes, and the desktop stamp badge column plus typed API client. Docs-Reviewed: doc-review stamp store + routes slice per document-review-surface.md
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds project-scoped document review persistence and authenticated API routes, then displays review badges for project files in the desktop file browser. Store, route, authorization, lifecycle, client API, and UI behavior receive test coverage. ChangesProject document review
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FilesApp
participant AuthMiddleware
participant project_doc_review
participant DocReviewStore
FilesApp->>AuthMiddleware: Fetch project review states
AuthMiddleware->>project_doc_review: Authorize review route
project_doc_review->>DocReviewStore: list_reviews(project_id)
DocReviewStore-->>project_doc_review: Review records
project_doc_review-->>FilesApp: Review state items
FilesApp->>FilesApp: Render file review badges
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
|
…ts.ts types - Add ReviewBadge component with onClick support for cycling review state - Add cycleDocReview callback for in-place state transitions - Wire badge into FileRow (list view) and grid cards (project: locations only) - Fetch review states on project-location navigation via projectsApi.docReviews.list - Remove duplicate DocReviewState/DocReview type definitions from projects.ts - Remove duplicate docReview API block; keep single canonical docReviews namespace - Use DocReview | DocReviewMissing union for GET response type - Apply encodeURIComponent to state filter query parameter Docs-Reviewed: doc-review stamp store + routes slice per document-review-surface.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
tests/test_doc_review.py (1)
231-242: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify actor recording for owner-session writes.
This test only writes
awaiting_review, which intentionally records no actor. Add an owner approval or changes-requested write and assert the corresponding actor equalsctx.uid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_doc_review.py` around lines 231 - 242, Extend test_owner_can_write_and_read with an owner approval or changes-requested write after the awaiting_review update, then assert the response records the expected review state and actor field equals ctx.uid. Keep the existing read/write assertions intact.tinyagentos/routes/project_doc_review.py (1)
70-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain the raised exceptions.
When raising an exception inside an
exceptblock, it is best practice to usefrom excto preserve the original traceback for debugging purposes.♻️ Proposed refactor
- except ValueError as exc: - detail = str(exc) - if "invalid transition" in detail: - raise HTTPException(status_code=409, detail=detail) - raise HTTPException(status_code=400, detail=detail) + except ValueError as exc: + detail = str(exc) + if "invalid transition" in detail: + raise HTTPException(status_code=409, detail=detail) from exc + raise HTTPException(status_code=400, detail=detail) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/project_doc_review.py` around lines 70 - 74, Update the ValueError handling block to chain both HTTPException raises from the caught exception using from exc, preserving the existing status codes and detail messages for invalid transitions and other validation errors.tinyagentos/projects/doc_review_store.py (1)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify
strict=Trueforzip().Explicitly declaring that
keysandrowmust be of equal length ensures safer iteration and satisfies static analysis (Ruff B905).♻️ Proposed refactor
- keys = [d[0] for d in description] - return dict(zip(keys, row)) + keys = [d[0] for d in description] + return dict(zip(keys, row, strict=True))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/projects/doc_review_store.py` around lines 38 - 39, Update the dict construction in the description-to-row mapping to call zip() with strict=True, ensuring keys and row must have equal lengths while preserving the existing mapping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/apps/FilesApp.tsx`:
- Around line 544-564: Update fetchReviewStates to map each review using
item.doc_path directly, removing the prefix construction and path parameter
because the endpoint returns project-root-relative paths. Update its callers,
including the useEffect dependency at line 674, to avoid refetching review
states on every directory navigation while preserving project-location changes
and error handling.
In `@desktop/src/lib/projects.ts`:
- Around line 81-95: Remove the earlier DocReviewState and DocReview type
declarations, retaining the later definitions around the existing
DocReviewMissing type. Ensure the remaining DocReview definition preserves its
non-nullable review_state while DocReviewMissing represents the nullable
variant.
- Around line 365-421: Consolidate the duplicated
projectsApi.docReview/docReviews definitions into one docReview namespace,
removing the duplicate object key and the separate docReviews property. Preserve
the complete get, set/setState, and list functionality from both definitions,
using the existing DocReview and DocReviewMissing/response types consistently
and retaining URL encoding and state query behavior.
In `@tests/test_doc_review.py`:
- Around line 327-338: Extend test_agent_other_project_is_404 to cover
cross-project document-review reads: issue a document GET and a document-list
GET against pid_b using token_a, and assert both return the same
existence-hiding 404 as the existing PUT case.
- Around line 40-166: Update the direct store tests around
test_get_missing_returns_none, test_awaiting_to_approved_records_actor,
test_awaiting_to_changes_requested_records_actor,
test_approved_back_to_awaiting_keeps_actor,
test_changes_requested_back_to_awaiting_keeps_actor,
test_invalid_transition_raises_value_error,
test_unknown_state_raises_value_error,
test_first_write_can_be_approved_or_changes_requested,
test_list_reviews_filters_by_state, and test_delete_review to use an async
fixture or equivalent try/finally cleanup that closes every initialized
DocReviewStore. Ensure both store and store2 in
test_invalid_transition_raises_value_error are closed even when assertions or
exceptions occur.
---
Nitpick comments:
In `@tests/test_doc_review.py`:
- Around line 231-242: Extend test_owner_can_write_and_read with an owner
approval or changes-requested write after the awaiting_review update, then
assert the response records the expected review state and actor field equals
ctx.uid. Keep the existing read/write assertions intact.
In `@tinyagentos/projects/doc_review_store.py`:
- Around line 38-39: Update the dict construction in the description-to-row
mapping to call zip() with strict=True, ensuring keys and row must have equal
lengths while preserving the existing mapping behavior.
In `@tinyagentos/routes/project_doc_review.py`:
- Around line 70-74: Update the ValueError handling block to chain both
HTTPException raises from the caught exception using from exc, preserving the
existing status codes and detail messages for invalid transitions and other
validation errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6241e0aa-defd-42de-943e-0e7c6ef5a46c
📒 Files selected for processing (8)
desktop/src/apps/FilesApp.tsxdesktop/src/lib/projects.tstests/test_doc_review.pytinyagentos/app.pytinyagentos/auth_middleware.pytinyagentos/projects/doc_review_store.pytinyagentos/routes/__init__.pytinyagentos/routes/project_doc_review.py
| @pytest.mark.asyncio | ||
| async def test_get_missing_returns_none(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| assert await store.get_review("p1", "a.md") is None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_awaiting_to_approved_records_actor(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| first = await store.set_review_state("p1", "a.md", "awaiting_review", "user-1") | ||
| assert first["review_state"] == "awaiting_review" | ||
| # First write to awaiting_review does not stamp an approver. | ||
| assert first["reviewed_by"] is None | ||
| assert first["changes_requested_by"] is None | ||
|
|
||
| approved = await store.set_review_state("p1", "a.md", "approved", "agent-7") | ||
| assert approved["review_state"] == "approved" | ||
| assert approved["reviewed_by"] == "agent-7" | ||
| assert approved["reviewed_at"] is not None | ||
| assert approved["changes_requested_by"] is None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_awaiting_to_changes_requested_records_actor(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| await store.set_review_state("p1", "b.md", "awaiting_review", "user-1") | ||
| r = await store.set_review_state("p1", "b.md", "changes_requested", "agent-3") | ||
| assert r["review_state"] == "changes_requested" | ||
| assert r["changes_requested_by"] == "agent-3" | ||
| assert r["changes_requested_at"] is not None | ||
| assert r["reviewed_by"] is None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_approved_back_to_awaiting_keeps_actor(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| await store.set_review_state("p1", "c.md", "awaiting_review", "u") | ||
| await store.set_review_state("p1", "c.md", "approved", "a1") | ||
| r = await store.set_review_state("p1", "c.md", "awaiting_review", "u") | ||
| assert r["review_state"] == "awaiting_review" | ||
| # Returning to awaiting_review does not clear the stamped approver. | ||
| assert r["reviewed_by"] == "a1" | ||
| assert r["reviewed_at"] is not None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_changes_requested_back_to_awaiting_keeps_actor(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| await store.set_review_state("p1", "c.md", "awaiting_review", "u") | ||
| await store.set_review_state("p1", "c.md", "changes_requested", "a2") | ||
| r = await store.set_review_state("p1", "c.md", "awaiting_review", "u") | ||
| assert r["review_state"] == "awaiting_review" | ||
| assert r["changes_requested_by"] == "a2" | ||
| assert r["changes_requested_at"] is not None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_invalid_transition_raises_value_error(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| await store.set_review_state("p1", "d.md", "awaiting_review", "u") | ||
| await store.set_review_state("p1", "d.md", "approved", "a1") | ||
| # approved -> changes_requested is not a permitted transition. | ||
| with pytest.raises(ValueError) as exc: | ||
| await store.set_review_state("p1", "d.md", "changes_requested", "a2") | ||
| assert "invalid transition" in str(exc.value) | ||
|
|
||
| # changes_requested -> approved is likewise invalid. | ||
| store2 = DocReviewStore(tmp_path / "projects2.db") | ||
| await store2.init() | ||
| await store2.set_review_state("p1", "e.md", "awaiting_review", "u") | ||
| await store2.set_review_state("p1", "e.md", "changes_requested", "a2") | ||
| with pytest.raises(ValueError) as exc2: | ||
| await store2.set_review_state("p1", "e.md", "approved", "a1") | ||
| assert "invalid transition" in str(exc2.value) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_unknown_state_raises_value_error(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| with pytest.raises(ValueError): | ||
| await store.set_review_state("p1", "f.md", "banana", "u") | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_first_write_can_be_approved_or_changes_requested(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| # A brand-new doc may be stamped approved (or changes_requested) directly. | ||
| approved = await store.set_review_state("p1", "g.md", "approved", "a1") | ||
| assert approved["review_state"] == "approved" | ||
| assert approved["reviewed_by"] == "a1" | ||
| cr = await store.set_review_state("p1", "h.md", "changes_requested", "a2") | ||
| assert cr["review_state"] == "changes_requested" | ||
| assert cr["changes_requested_by"] == "a2" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_list_reviews_filters_by_state(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| await store.set_review_state("p1", "x.md", "awaiting_review", "u") | ||
| await store.set_review_state("p1", "y.md", "approved", "a1") | ||
| await store.set_review_state("p1", "z.md", "awaiting_review", "u") | ||
|
|
||
| approved = await store.list_reviews("p1", state="approved") | ||
| assert [r["doc_path"] for r in approved] == ["y.md"] | ||
|
|
||
| all_reviews = await store.list_reviews("p1") | ||
| assert len(all_reviews) == 3 | ||
| assert {r["doc_path"] for r in all_reviews} == {"x.md", "y.md", "z.md"} | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_delete_review(tmp_path): | ||
| store = DocReviewStore(tmp_path / "projects.db") | ||
| await store.init() | ||
| await store.set_review_state("p1", "k.md", "awaiting_review", "u") | ||
| assert await store.delete_review("p1", "k.md") is True | ||
| assert await store.get_review("p1", "k.md") is None | ||
| assert await store.delete_review("p1", "missing.md") is False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the stores created by direct store tests.
Lines 40-166 repeatedly call store.init() without store.close(), leaving database handles open. Use an async fixture with try/finally to close each store, including store2.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_doc_review.py` around lines 40 - 166, Update the direct store
tests around test_get_missing_returns_none,
test_awaiting_to_approved_records_actor,
test_awaiting_to_changes_requested_records_actor,
test_approved_back_to_awaiting_keeps_actor,
test_changes_requested_back_to_awaiting_keeps_actor,
test_invalid_transition_raises_value_error,
test_unknown_state_raises_value_error,
test_first_write_can_be_approved_or_changes_requested,
test_list_reviews_filters_by_state, and test_delete_review to use an async
fixture or equivalent try/finally cleanup that closes every initialized
DocReviewStore. Ensure both store and store2 in
test_invalid_transition_raises_value_error are closed even when assertions or
exceptions occur.
| async def test_agent_other_project_is_404(self, ctx): | ||
| pid_a = await _new_project(ctx, "alpha") | ||
| pid_b = await _new_project(ctx, "bravo") | ||
| _cid, token_a = await _mint_agent(ctx, pid_a) | ||
| async with _bare(ctx.app) as bare: | ||
| resp = await bare.put( | ||
| f"/api/projects/{pid_b}/doc-review/README.md", | ||
| json={"state": "awaiting_review"}, | ||
| headers=_hdr(token_a), | ||
| ) | ||
| # Existence-hiding 404, identical to a non-owner session. | ||
| assert resp.status_code == 404, resp.text |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cover cross-project reads as well as writes.
The stated isolation invariant applies to all document-review routes, but this test only exercises PUT. Add cross-project document GET and list GET cases expecting the same existence-hiding 404.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_doc_review.py` around lines 327 - 338, Extend
test_agent_other_project_is_404 to cover cross-project document-review reads:
issue a document GET and a document-list GET against pid_b using token_a, and
assert both return the same existence-hiding 404 as the existing PUT case.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| async (docPath: string, current: string | null) => { | ||
| if (!isProjectLocation(location)) return; | ||
| const pid = projectSlug(location); | ||
| const order: DocReviewState[] = ["awaiting_review", "approved", "changes_requested"]; |
There was a problem hiding this comment.
WARNING: Doc-review cycle includes an invalid transition (approved -> changes_requested)
The backend state machine (VALID_TRANSITIONS in tinyagentos/projects/doc_review_store.py) only permits approved -> awaiting_review. This cycle order is [awaiting_review, approved, changes_requested], so clicking the badge while a doc is approved attempts approved -> changes_requested, which the route rejects with HTTP 409. That error is swallowed by the catch block below, so the badge silently stays "Approved" and appears stuck.
No linear 3-state cycle is possible because approved and changes_requested are not connected to each other (both only return to awaiting_review). Use a per-state next mapping instead, e.g.:
const NEXT_STATE: Record<DocReviewState, DocReviewState> = {
awaiting_review: "approved",
approved: "awaiting_review",
changes_requested: "awaiting_review",
};
const next = NEXT_STATE[(current as DocReviewState) ?? "awaiting_review"];
(Note this still cannot reach changes_requested from the badge - you may want a dedicated affordance for that state.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const nextMap: Record<string, string | null> = { ...prev, [docPath]: next }; | ||
| return nextMap; | ||
| }); | ||
| } catch { |
There was a problem hiding this comment.
SUGGESTION: Surface rejected writes to the user instead of silently swallowing
The catch {} discards every failure (403/409/401/offline), so when a write is rejected the badge simply does not change with no explanation. Consider calling setError(...) (or re-throwing) so the user knows the action was not applied.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| sets: list[str] = ["review_state = ?", "updated_at = ?"] | ||
| params: list = [new_state, now] | ||
|
|
||
| if new_state == "approved": |
There was a problem hiding this comment.
SUGGESTION: Opposite actor columns are not cleared on transition
When moving into approved you set reviewed_by/reviewed_at but never clear changes_requested_by/changes_requested_at, and vice versa. Because transitions route back through awaiting_review (e.g. approved -> awaiting_review -> changes_requested), a doc can end in changes_requested while still carrying a stale reviewed_by from the earlier approval. Consider clearing the non-applicable actor columns on each transition for cleaner audit semantics.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL(none) WARNING
SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 93.2K · Output: 32.8K · Cached: 802.1K |
|
Superseded by #1835 (same branch, complete work + tests). |
1 participant
Part of #1802 (document-review-surface). Slice 3: per-document doc-review stamp store, project-scoped agent-token gated routes, desktop stamp badge, and tests.
tinyagentos/projects/doc_review_store.py: review_state machine (awaiting_review/approved/changes_requested) with actor recording and timestamps.tinyagentos/routes/project_doc_review.py: PUT/GET/api/projects/{project_id}/doc-review/{doc_path}, agent-scope gated writes, 409 on invalid transitions.tinyagentos/app.pybeside the other projects stores; router registered intinyagentos/routes/__init__.py.tinyagentos/auth_middleware.py: allow agent bearer tokens to reach the doc-review routes.desktop/src/apps/FilesApp.tsx+desktop/src/lib/projects.ts: stamp badge column and typed API client.tests/test_doc_review.py: state machine, actor recording, 409s, agent-scope gating.DO NOT MERGE
Summary by CodeRabbit
New Features
Bug Fixes