feat(projects): members canvas checkboxes + exclusive Lead (#1800 slice 6)#1828
feat(projects): members canvas checkboxes + exclusive Lead (#1800 slice 6)#1828jaylfc wants to merge 1 commit into
Conversation
…ce 6)
Implement slice 6 of the lead-agent identity and canvas access epic.
Backend:
- projects gains lead_member_id; backfill it from the single is_lead flag
and retire set_member_lead.
- Add PATCH /api/projects/{project_id}/lead to set or clear the exclusive
lead; remove the old per-member lead route.
- The a2a quiet-filter and settings.leads sync read lead_member_id.
Frontend:
- ProjectMembers gains a second Can read canvas checkbox and an exclusive
Lead selector driven by lead_member_id.
- projects.setLead is now project-level and canvas-api.setPermission takes a
read vs edit flag.
Tests cover exclusive lead promotion and clearing, 404 on non-member,
session-only gating, a2a quiet-filter sync, and the selector/read checkbox.
Docs-Reviewed: members canvas checkboxes + exclusive lead per lead-agent-identity-and-canvas-access.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? |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| project_id, user.user_id, "project.lead_changed", {"member_id": body.member_id} | ||
| ) | ||
| members = await store.list_members(project_id) | ||
| _mirror(request, {**p, "members": members}) |
There was a problem hiding this comment.
WARNING: Filesystem mirror is written with the stale, pre-change lead_member_id
p is fetched at line 318, before set_lead updates the DB, so {**p, "members": members} still carries the old lead_member_id. write_project_yaml (folders.py:28) serializes the entire payload, so the on-disk project.yaml will reflect the previous lead after every change. The DB remains authoritative, but any consumer reading project.yaml (e.g. agent/file access) sees a divergent lead. Re-fetch the project after set_lead (or patch p["lead_member_id"]) before calling _mirror.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| (val, project_id, member_id), | ||
| async def _backfill_lead_member_id(self) -> None: | ||
| rows = await (await self._db.execute( | ||
| "SELECT id FROM projects WHERE lead_member_id IS NULL" |
There was a problem hiding this comment.
WARNING: Backfill can resurrect a deliberately-cleared lead
The backfill keys only on lead_member_id IS NULL and re-derives the lead from the legacy is_lead = 1 flag. set_lead never clears is_lead, so after a user intentionally clears the lead (set_lead(None) → lead_member_id = NULL), the next process restart re-runs _backfill_lead_member_id, finds flagged of length 1, and re-populates lead_member_id from the stale flag — silently undoing the user's "No lead" choice. Track migration state separately (e.g. a lead_migrated flag, or clear is_lead during migration) so intentional clears are not reverted.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| class LeadIn(BaseModel): | ||
| is_lead: bool | ||
| class ProjectLeadIn(BaseModel): | ||
| member_id: "str | None" = None |
There was a problem hiding this comment.
SUGGESTION: An empty request body silently clears the lead
member_id defaults to None, so a PATCH /api/projects/{pid}/lead with an empty {} body clears the lead instead of erroring. Consider making member_id required (no default) so an unintended empty payload is rejected with 422 rather than wiping the lead.
| member_id: "str | None" = None | |
| member_id: "str | None" |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_set_lead_exclusive_replaces_previous(client): |
There was a problem hiding this comment.
SUGGESTION: Test name/body overclaims exclusivity coverage
test_set_lead_exclusive_replaces_previous never sets two distinct members: it sets a1, clears it, then sets a1 again. It does not actually verify that promoting a second member unsets the first. Cross-member exclusivity is only asserted in tests/test_routes_projects.py::test_set_lead_exclusive_leaves_only_last. Rename this test or add a second distinct member so the body matches the docstring.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // Revert on failure so the UI stays consistent with the server. | ||
| setLeadMemberId(project.lead_member_id ?? null); | ||
| } | ||
| onChanged(); |
There was a problem hiding this comment.
SUGGESTION: onChanged() fires even when the lead change failed
After the catch reverts local leadMemberId state, onChanged() is still called unconditionally, triggering an unnecessary parent refresh/flicker on failure. Consider calling onChanged() only on success (or moving it inside the try) so a failed request doesn't cause a spurious re-render.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (11 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 100.5K · Output: 16.6K · Cached: 441.7K |
|
Superseded by #1838, which landed all seven slices as one consistent set on dev (with the CI-caught active-handle migration-order fix folded). Closing. |
1 participant
DO NOT MERGE
Summary by Gitar
is_leadflag with a single, project-levellead_member_idpointer (D7) to enforce an exclusive "one lead per project" invariant.is_leadflags to the newlead_member_idcolumn.PATCH /api/projects/{pid}/members/{mid}/leadtoPATCH /api/projects/{pid}/lead.can_read_canvasandcan_edit_canvascheckboxes for finer granularity.canvasApi.setPermissionto handle specific capability flags instead of a single binary toggle.selectelement inProjectMembersto manage the exclusive project lead directly.This will update automatically on new commits.