Repository navigation
Various hot fixes - #135
Conversation
Every org gets a default auto-approve policy (backfill plus an orgs insert trigger). Repo override policies now cascade when a repo is deleted, so deleting a connector no longer fails. LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
Reviews started from New Review or the Explore PR table now get the policy check and Auto Approve tab. Trigger Review in Explore opens the new review; the PR description links to repo and org policy under the decisions table. LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
New Review searches open PRs by repo, title or author and lists other ways to review. All back buttons use one Icons.Back arrow, placed top left and aligned with the title. LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
Implement Default Auto-Approve Policies and Policy Check TriggeringOverviewThis change implements default auto-approve policies for all organizations. It also centralizes logic to trigger policy checks for reviews. This ensures consistent auto-approval behavior. Technical Highlights
Impact
|
|
|
||
| -- Deleting a connector cascades to its repositories; repo override policies blocked that. | ||
| -- A repo's override policy is meaningless without the repo, so delete it too. | ||
| ALTER TABLE auto_approve_policies DROP CONSTRAINT auto_approve_policies_repository_id_fkey; |
There was a problem hiding this comment.
Severity: info
The existing foreign key constraint has been removed.
Suggestions:
- Consider adding a comment explaining why the existing constraint is being dropped.
| -- Deleting a connector cascades to its repositories; repo override policies blocked that. | ||
| -- A repo's override policy is meaningless without the repo, so delete it too. | ||
| ALTER TABLE auto_approve_policies DROP CONSTRAINT auto_approve_policies_repository_id_fkey; | ||
| ALTER TABLE auto_approve_policies ADD CONSTRAINT auto_approve_policies_repository_id_fkey |
There was a problem hiding this comment.
Severity: info
A new foreign key constraint has been added with ON DELETE CASCADE.
Suggestions:
- Ensure
repositories.idis indexed for efficient cascade operations.
| -- migrate:up | ||
|
|
||
| -- Auto-approve is on by default: every org gets an org-level policy with the column defaults. | ||
| INSERT INTO auto_approve_policies (org_id) |
There was a problem hiding this comment.
Severity: info
The default organization-level policy has been inserted.
Suggestions:
- Consider adding a unique constraint on
(org_id, repository_id)to prevent duplicates.
| SELECT 1 FROM auto_approve_policies p WHERE p.org_id = o.id AND p.repository_id IS NULL | ||
| ); | ||
|
|
||
| CREATE FUNCTION create_default_auto_approve_policy() RETURNS trigger |
There was a problem hiding this comment.
Severity: info
A function for the default policy has been created.
Suggestions:
- Consider naming the function more descriptively, e.g.,
ensure_default_auto_approve_policy.
| LANGUAGE plpgsql | ||
| AS $$ | ||
| BEGIN | ||
| INSERT INTO auto_approve_policies (org_id) VALUES (NEW.id); |
There was a problem hiding this comment.
Severity: info
The default policy for new organizations has been inserted.
Suggestions:
- Ensure
orgs.idis indexed for efficient trigger operation.
| -- Name: create_default_auto_approve_policy(); Type: FUNCTION; Schema: public; Owner: - | ||
| -- | ||
|
|
||
| CREATE FUNCTION public.create_default_auto_approve_policy() RETURNS trigger |
There was a problem hiding this comment.
Severity: info
This is the function definition for the default policy.
Suggestions:
- Consider adding a comment explaining the purpose of this function.
| -- Name: orgs orgs_default_auto_approve_policy; Type: TRIGGER; Schema: public; Owner: - | ||
| -- | ||
|
|
||
| CREATE TRIGGER orgs_default_auto_approve_policy AFTER INSERT ON public.orgs FOR EACH ROW EXECUTE FUNCTION public.create_default_auto_approve_policy(); |
There was a problem hiding this comment.
Severity: info
A trigger has been added to create the default policy for new organizations.
Suggestions:
- Consider adding a comment explaining the purpose of this trigger.
|
|
||
| ALTER TABLE ONLY public.auto_approve_policies | ||
| ADD CONSTRAINT auto_approve_policies_repository_id_fkey FOREIGN KEY (repository_id) REFERENCES public.repositories(id); | ||
| ADD CONSTRAINT auto_approve_policies_repository_id_fkey FOREIGN KEY (repository_id) REFERENCES public.repositories(id) ON DELETE CASCADE; |
There was a problem hiding this comment.
Severity: info
This is a foreign key constraint with ON DELETE CASCADE.
Suggestions:
- Verify that
repositories.idis a primary key or has a unique index.
| ('20261012100000'), | ||
| ('20261013090000'); | ||
| ('20261013090000'), | ||
| ('20261017090000'), |
There was a problem hiding this comment.
Severity: info
New migration versions have been added.
Suggestions:
- Ensure migration versions are sequential and unique.
| return c.JSON(http.StatusInternalServerError, ErrorResponse{Error: err.Error()}) | ||
| } | ||
| s.enrichMetadata(ctx) | ||
| if req.FromPRLink { |
There was a problem hiding this comment.
Severity: info
The inline SQL has been removed and replaced by a helper function.
Suggestions:
- Add a comment to
markForPolicyCheckexplaining thepull_request_id IS NOT NULLcondition.
| func (s *Server) markForPolicyCheck(reviewID, orgID int64) { | ||
| if _, err := s.db.Exec( | ||
| `UPDATE reviews SET metadata = COALESCE(metadata, '{}'::jsonb) || '{"pr_link_policy": true}'::jsonb | ||
| WHERE id = $1 AND org_id = $2 AND pull_request_id IS NOT NULL`, |
There was a problem hiding this comment.
Severity: info
A condition has been added to only update reviews that are linked to a pull request.
Suggestions:
- Consider logging a warning if
pull_request_idis NULL for a review that is being processed.
|
|
||
| // Phase 4: Enrich metadata (fetch MR details, update DB) | ||
| s.enrichMetadata(ctx) | ||
| s.markForPolicyCheck(ctx.review.ID, ctx.orgID) |
There was a problem hiding this comment.
Severity: info
A call has been added to mark the review for a policy check.
Suggestions:
- Ensure
ctx.orgIDis always populated before this call.
|
|
||
| - Search/filter/sort PRs by provider, repo, state, etc. | ||
| - Trigger a review directly for a specific pull request | ||
| (`triggerReviewForPullRequest`). |
There was a problem hiding this comment.
Severity: info
The description has been updated to include information about the policy check.
Suggestions:
- Consider adding a link to the 'Auto Approve Policies' settings page for more details.
| ## Purpose | ||
|
|
||
| Form to manually trigger a code review (as opposed to it firing from a git | ||
| webhook or the `lrc` CLI hook). Lets the user pick a connected AI provider |
There was a problem hiding this comment.
Severity: info
The description for manual reviews has been updated.
Suggestions:
- Clarify what 'synced' means in the context of PR LiveReview.
| - Type a repository name, PR title or author to see matching open PRs. Click | ||
| one, or use ↑/↓ and Enter, to start its review. | ||
| - Or paste a PR/MR URL and click **Start review**. Either way the new review's | ||
| page opens right away, and for a PR LiveReview already synced the org's |
There was a problem hiding this comment.
Severity: info
The policy check process for pull request reviews has been clarified.
Suggestions:
- Ensure the link to the 'Explore → Merge Requests' page is accurate.
| opens the review's detail page. | ||
| - **Review history** → the [Reviews list](reviews-list.md) filtered to that | ||
| PR (`/reviews?pull_request_id=<id>`). | ||
| - A line under the auto-approval decisions (or under the two links before any |
There was a problem hiding this comment.
Severity: info
Information regarding policy customization links has been added.
Suggestions:
- Ensure the links to repository and organization settings are correct and accessible.
| - The decisions table shows the last 10 policy-checked reviews. | ||
| - Reviews started any other way (reviewer assignment, @mentions, Explore) don't | ||
| run the policy check. | ||
| - Reviews started by hand for a known PR also run the policy check and get the |
There was a problem hiding this comment.
Severity: info
It has been clarified which review triggers the policy check.
Suggestions:
- Consider adding a note about how to explicitly trigger a policy check for other review types if needed.
| legacySectionEnd = "<!-- livereview:end -->" | ||
| ) | ||
|
|
||
| // postLinksComment adds the "Start a review / Review history" links to the end of a new PR/MR's description. |
There was a problem hiding this comment.
Severity: info
The comment has been updated to reflect the new links.
Suggestions:
- Ensure the comment accurately describes all links added by the function.
|
|
||
| info, err := loadRepositoryConnectorInfo(w.db, repositoryID) | ||
| if err == nil { | ||
| err = writePRSection(ctx, info, number, buildPRLinksSection(host, pullRequestID, "")) |
There was a problem hiding this comment.
Severity: info
The repositoryID has been passed to the buildPRLinksSection function.
Suggestions:
- Consider adding a unique constraint on
(repository_id, pull_request_id)in thereviewstable if not already present.
| } | ||
|
|
||
| // buildPRLinksSection renders the LiveReview section; history is the review history table, or "". | ||
| func buildPRLinksSection(host string, pullRequestID int64, history string) string { |
There was a problem hiding this comment.
Severity: info
The repositoryID parameter has been added.
Suggestions:
- Ensure the
repositoryIDparameter is always valid and non-zero when passed.
| - ▶️ **[Start a review](%s/#/pr/%d/review)**: reviews all changes in this PR against its target branch | ||
| - 📜 **[Review history](%s/#/reviews?pull_request_id=%d)**: all past reviews of this PR | ||
|
|
||
| %s%s |
There was a problem hiding this comment.
Severity: info
Links for policy customization have been added.
Suggestions:
- Ensure the URLs for repository and organization settings are correctly formatted and resolve to the intended pages.
| } | ||
| b.WriteString("\n") | ||
| return writePRSection(ctx, info, number, buildPRLinksSection(host, pullRequestID, b.String())) | ||
| return writePRSection(ctx, info, number, buildPRLinksSection(host, pullRequestID, info.RepositoryID, b.String())) |
There was a problem hiding this comment.
Severity: info
The repositoryID has been passed to the buildPRLinksSection function.
Suggestions:
- Verify that
info.RepositoryIDis correctly populated before this call.
| // already inserted a row for this org by the time the test ends; its FK | ||
| // has no cascade, so it must be cleared first or the org delete leaks. | ||
| _, _ = db.Exec(`DELETE FROM dashboard_cache WHERE org_id = $1`, orgID) | ||
| _, _ = db.Exec(`DELETE FROM auto_approve_policies WHERE org_id = $1`, orgID) |
There was a problem hiding this comment.
Severity: info
The default policy for the organization has been deleted.
Suggestions:
- Consider using
TRUNCATE TABLE auto_approve_policiesif it's safe for all tests.
|
|
||
| func insertPolicy(t *testing.T, db *sql.DB, orgID int64, repoID *int64, mode string, minLevel int) int64 { | ||
| var id int64 | ||
| if repoID == nil { |
There was a problem hiding this comment.
Severity: info
The default organization-level policy has been updated.
Suggestions:
- Ensure the
repository_id IS NULLcondition correctly identifies the default policy.
| func TestResolvePolicyNoneConfigured(t *testing.T) { | ||
| db := testDB(t) | ||
| orgID := seedOrg(t, db, "policy-resolve-none") | ||
| // Drop the default row the org-insert trigger created. |
There was a problem hiding this comment.
Severity: info
The default policy row has been deleted.
Suggestions:
- Verify that this deletion does not affect other tests that rely on the default policy.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
The button icon has been changed to 'Back'.
Suggestions:
- Ensure the
Icons.Backcomponent is correctly imported and styled.
| } | ||
|
|
||
| // buildPRLinksSection renders the LiveReview section; history is the review history table, or "". | ||
| func buildPRLinksSection(host string, pullRequestID int64, history string) string { |
There was a problem hiding this comment.
Severity: info
Add the repositoryID parameter.
Suggestions:
- Function signature updated to accommodate new link generation.
| - ▶️ **[Start a review](%s/#/pr/%d/review)**: reviews all changes in this PR against its target branch | ||
| - 📜 **[Review history](%s/#/reviews?pull_request_id=%d)**: all past reviews of this PR | ||
|
|
||
| %s%s |
There was a problem hiding this comment.
Severity: info
Add repository and organization policy links.
Suggestions:
- This adds valuable links for users to customize their approval policies.
| } | ||
| b.WriteString("\n") | ||
| return writePRSection(ctx, info, number, buildPRLinksSection(host, pullRequestID, b.String())) | ||
| return writePRSection(ctx, info, number, buildPRLinksSection(host, pullRequestID, info.RepositoryID, b.String())) |
There was a problem hiding this comment.
Severity: info
Pass the repositoryID to the buildPRLinksSection function.
Suggestions:
- Ensures the function receives all necessary parameters for its updated functionality.
| // already inserted a row for this org by the time the test ends; its FK | ||
| // has no cascade, so it must be cleared first or the org delete leaks. | ||
| _, _ = db.Exec(`DELETE FROM dashboard_cache WHERE org_id = $1`, orgID) | ||
| _, _ = db.Exec(`DELETE FROM auto_approve_policies WHERE org_id = $1`, orgID) |
There was a problem hiding this comment.
Severity: info
Delete the default organization policy row.
Suggestions:
- Consider if this explicit deletion is necessary if the trigger logic guarantees a default row.
|
|
||
| func insertPolicy(t *testing.T, db *sql.DB, orgID int64, repoID *int64, mode string, minLevel int) int64 { | ||
| var id int64 | ||
| if repoID == nil { |
There was a problem hiding this comment.
Severity: info
Update the default organization policy if repoID is null.
Suggestions:
- This logic duplicates the INSERT logic but with an UPDATE. Consider a single upsert pattern if possible.
| func TestResolvePolicyNoneConfigured(t *testing.T) { | ||
| db := testDB(t) | ||
| orgID := seedOrg(t, db, "policy-resolve-none") | ||
| // Drop the default row the org-insert trigger created. |
There was a problem hiding this comment.
Severity: info
Delete the default organization policy row.
Suggestions:
- Explicitly delete default policy for test isolation.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
The icon has been changed from 'Add' to 'Back'.
Suggestions:
- Use consistent navigation icons across components.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
Apply a negative margin for alignment purposes.
Suggestions:
- Ensure consistent spacing and alignment for navigation elements.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
The icon has been changed from 'Add' to 'Back'.
Suggestions:
- Use consistent navigation icons across components.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
Apply a negative margin for alignment purposes.
Suggestions:
- Ensure consistent spacing and alignment for navigation elements.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
The icon has been changed from 'Add' to 'Back'.
Suggestions:
- Use consistent navigation icons across components.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
Apply a negative margin for alignment purposes.
Suggestions:
- Ensure consistent spacing and alignment for navigation elements.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
The icon has been changed from 'Add' to 'Back'.
Suggestions:
- Use consistent navigation icons across components.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
Apply a negative margin for alignment purposes.
Suggestions:
- Ensure consistent spacing and alignment for navigation elements.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
The icon has been changed from 'Add' to 'Back'.
Suggestions:
- Use consistent navigation icons across components.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
Apply a negative margin for alignment purposes.
Suggestions:
- Ensure consistent spacing and alignment for navigation elements.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
The icon has been changed from 'Add' to 'Back'.
Suggestions:
- Use consistent navigation icons across components.
| return ( | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center"> | ||
| <Button variant="ghost" icon={<Icons.Add />} onClick={() => navigate('/git')} iconPosition="left" className="text-sm"> |
There was a problem hiding this comment.
Severity: info
Apply a negative margin for alignment purposes.
Suggestions:
- Ensure consistent spacing and alignment for navigation elements.
| </svg> | ||
| ), | ||
|
|
||
| Back: () => ( |
There was a problem hiding this comment.
Severity: info
Add a new 'Back' icon component.
Suggestions:
- Centralizes common UI elements for consistency.
| @@ -1,4 +1,5 @@ | |||
| import React, { useState, useEffect } from 'react'; | |||
| import { Icons } from '../../components/UIPrimitives'; | |||
There was a problem hiding this comment.
Severity: info
Import icons for reusable components.
Suggestions:
- Leverages shared UI primitives for consistency.
| <div className="flex justify-end"> | ||
| <Button | ||
| variant="ghost" | ||
| icon={<Icons.Back />} |
There was a problem hiding this comment.
Severity: info
Use the reusable 'Back' icon.
Suggestions:
- Standardizes navigation elements using shared primitives.
| @@ -96,7 +96,6 @@ const MergeRequests: React.FC = () => { | |||
| // In-memory only (not persisted) - reverts to "Trigger Review" on a page | |||
There was a problem hiding this comment.
Severity: info
Remove the triggeredReviewIds state.
Suggestions:
- Simplifies state management by removing unused state.
| const response = await triggerReviewForPullRequest(pr.id); | ||
| setTriggeredReviewIds((prev) => ({ ...prev, [pr.id]: response.reviewId })); | ||
| notify.success(`Review triggered for #${pr.number}`); | ||
| navigate(`/reviews/${response.reviewId}`); |
There was a problem hiding this comment.
Severity: info
Navigate directly to the review page.
Suggestions:
- Streamlines user flow by redirecting immediately after action.
| @@ -316,22 +314,6 @@ const MergeRequests: React.FC = () => { | |||
| header: () => <span className="font-semibold text-slate-300 uppercase tracking-wide text-xs">Actions</span>, | |||
There was a problem hiding this comment.
Severity: info
Remove the logic associated with the 'See Logs' button.
Suggestions:
- Removes UI element tied to deprecated state.
| }, | ||
| }, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| ], [triggeringId, triggeredReviewIds, navigate]); |
There was a problem hiding this comment.
Severity: info
Remove triggeredReviewIds from the dependency array.
Suggestions:
- Ensures effect dependencies are accurate after state removal.
| @@ -1,6 +1,6 @@ | |||
| import React, { useEffect, useState } from 'react'; | |||
| import { Link, useLocation, useNavigate, useParams } from 'react-router-dom'; | |||
| import { LuArrowLeft, LuDatabase } from 'react-icons/lu'; | |||
There was a problem hiding this comment.
Severity: info
Remove the LuArrowLeft import.
Suggestions:
- Unused icon import removed.
| }, [repoId]); | ||
|
|
||
| const back = ( | ||
| <Link to="/explore/repositories" className="mb-4 inline-flex items-center gap-1.5 text-sm text-slate-300 hover:text-white"> |
There was a problem hiding this comment.
Severity: info
Use a Button component with a Link and the reusable 'Back' icon.
Suggestions:
- Standardizes navigation elements using shared primitives.
| }, [repoId]); | ||
|
|
||
| const back = ( | ||
| <Link to="/explore/repositories" className="mb-4 inline-flex items-center gap-1.5 text-sm text-slate-300 hover:text-white"> |
There was a problem hiding this comment.
Severity: info
Apply a negative margin for alignment purposes.
Suggestions:
- Ensure consistent spacing and alignment for navigation elements.
| <div className="flex items-center mb-6"> | ||
| <Button | ||
| variant="ghost" | ||
| icon={<Icons.Dashboard />} |
There was a problem hiding this comment.
Severity: info
The icon has been changed from 'Dashboard' to 'Back'.
Suggestions:
- Use consistent navigation icons across components.
Summary
Fixes and polish found while testing the auto-approve release (v1.0.11) on prod and the self-hosted instance.
Auto-approve
orgscovers new ones. Before this, the settings page only showed defaults, nothing was saved, and reviews got "No policy evaluation".Fixes
auto_approve_policies.repository_idhad noON DELETE CASCADE. A new migration adds it.UI
Icons.Backarrow, top left and lined up with the page title, across connectors, connector details, review detail, repo settings, scheduled pages, CLI/MCP pages, checkout and licenses. Some of these used a plus or dashboard icon before.new-review.md,pr-review-link.md,merge-requests.md.Linked Issue
None. These came out of prod and self-hosted testing of v1.0.11.
Validation
go build ./internal/...andgo vet(internal/api,internal/jobqueue,internal/policyengine) pass.npx tsc --noEmitpasses.go test ./internal/policyengine/(the test helpers were updated for the new trigger).DELETE /connectors/66succeed. The migration itself wasn't run there.Checklist
SECURITY.mdand updated related documentation if needed.Notes For Reviewers
auto_approve_policies.org_idstill has no cascade, so a hard org delete would now be blocked by the default row. App code only soft-deletes orgs. Test cleanups in other packages delete orgs with_, _ =, so they will quietly leave test orgs behind.#12) doesn't match, because the backend searches only title, author and repo.DeleteConnectoraccepts API keys, although the project rules say destructive endpoints should be session-only. This isn't changed here.🔍 LiveReview