Repository navigation
feat: redesign Auto Approve panel, move PR controls to top, show review time in local zone - #147
Conversation
Add User Time Zone SupportOverviewThis change introduces user time zone preferences. It stores time zone data, provides an API for updates, and automatically syncs client-side. This enables displaying time-sensitive information, like review timestamps, in the user's local time. Technical Highlights
Impact
|
![livereview-agent[bot]](https://avatars.githubusercontent.com/in/5380735?s=60&v=4){kind=small}
| @@ -0,0 +1,5 @@ | |||
| -- migrate:up | |||
| ALTER TABLE users ADD COLUMN IF NOT EXISTS time_zone TEXT; | |||
There was a problem hiding this comment.
Severity: warning
You should add a NOT NULL constraint if the time_zone field is always required, or alternatively, provide a default value for it.
Suggestions:
- Consider adding
NOT NULLconstraint and a default value (e.g., 'UTC') if a time zone is always expected for users. - If
NULLis acceptable, ensure application logic handlesNULLtime zones gracefully (e.g., falling back to UTC).
| if err := c.Bind(&req); err != nil { | ||
| return echo.NewHTTPError(http.StatusBadRequest, "Invalid request body") | ||
| } | ||
| if _, err := time.LoadLocation(req.TimeZone); err != nil || req.TimeZone == "" || req.TimeZone == "Local" { |
There was a problem hiding this comment.
Severity: warning
It would be beneficial to validate the length of req.TimeZone to prevent excessively long strings from being submitted.
Suggestions:
- Add a length check for
req.TimeZoneto prevent potential denial-of-service or storage issues with very long strings.
| // ponytail: only the last 10 rows, so the table stays under description size limits (Azure DevOps: 4000 chars). | ||
| `SELECT id, policy, forced_reason FROM ( | ||
| SELECT r.id, r.metadata->'policy_result' AS policy, | ||
| `SELECT id, created_at, policy, forced_reason, time_zone FROM ( |
There was a problem hiding this comment.
Severity: warning
Please add r.user_email to the select list for the time_zone subquery.
Suggestions:
- Add
r.user_emailto theSELECTclause of the outer query. Thetime_zonesubquery needsr.user_emailto link to theuserstable, butr.user_emailis not available in the outer query'sSELECTlist. This will cause a SQL error.
| func formatReviewTime(t time.Time, timeZone sql.NullString) string { | ||
| if timeZone.Valid { | ||
| if loc, err := time.LoadLocation(timeZone.String); err == nil { | ||
| return t.In(loc).Format("2 Jan 2006, 15:04 MST") |
There was a problem hiding this comment.
Severity: info
Consider using time.RFC822 or time.RFC1123Z for a more standard date and time format.
Suggestions:
- Use a standard time format constant like
time.RFC822ortime.RFC1123Zfor consistency and easier parsing if this output is consumed by other systems. - Ensure the chosen format is clear and unambiguous for users across different locales.
| section = strings.TrimRight(section, "\n") | ||
| j := strings.Index(body[i:], end) | ||
| if j < 0 { | ||
| return body[:i] + strings.TrimRight(section, "\n"), true |
There was a problem hiding this comment.
Severity: warning
You need to handle the case where the end marker is missing after the start marker has been found.
Suggestions:
- If
j < 0, theendmarker is missing. The current logicreturn body[:i] + strings.TrimRight(section, "\n"), trueeffectively truncates thebodyafter thestartmarker and appends the newsection. This might silently delete content if theendmarker is expected but absent. Consider logging a warning or returningfalseforokin this specific case to indicate a partial replacement.
| return strings.TrimRight(next, "\n") + "\n" | ||
| } | ||
| if i := strings.Index(body, legacySectionStart); i >= 0 { | ||
| return strings.TrimRight(body[:i], "\n") + "\n" |
There was a problem hiding this comment.
Severity: warning
Ensure that body[:i] does not inadvertently remove content that appears before legacySectionStart.
Suggestions:
- The
removeOldSectionfunction forlegacySectionStart(if i := strings.Index(body, legacySectionStart); i >= 0 { return strings.TrimRight(body[:i], "\n") + "\n" }) removes everything after the start of the legacy section. This assumes the legacy section was always at the very end of the PR description. If any content could appear after the legacy section but before the new sections, it would be deleted. Confirm this assumption is safe.
| if strings.TrimSpace(body) == "" { | ||
| return section | ||
| } | ||
| return section + "\n" + strings.TrimLeft(body, "\n") |
There was a problem hiding this comment.
Severity: warning
Make sure that strings.TrimLeft(body, "\n") does not remove more newlines than intended.
Suggestions:
- If
bodystarts with multiple newlines,strings.TrimLeft(body, "\n")will remove all of them. This might lead to less vertical spacing than desired between the new controls section and the original PR body. Considerstrings.TrimLeft(body, "\n")orstrings.TrimSpace(body)if the intent is to normalize leading newlines, but be aware of potential formatting changes.
| className={`grid grid-cols-[1.25rem_1fr] items-center gap-x-3 gap-y-2 text-sm md:grid-cols-[1.25rem_11rem_1fr_auto] ${canExpand ? 'cursor-pointer' : ''}`} | ||
| onClick={(e) => { | ||
| // Links and the Details button handle their own clicks. | ||
| if (canExpand && !(e.target as HTMLElement).closest('a,button')) toggle(); |
There was a problem hiding this comment.
Severity: warning
Regarding the click handler logic, please ensure that e.target.closest('a,button') correctly identifies clicks on nested interactive elements.
Suggestions:
- Thoroughly test click behavior for links/buttons inside expandable rows.
| <span className="hidden w-[5.5rem] md:block" aria-hidden="true" /> | ||
| )} | ||
| {canManageCurrentOrg && st.stage === 'eligibility' && viewerId && ( | ||
| <Link to={`/settings/users/edit/${viewerId}`} className={actionClass} title="Open your user settings (review level)"> |
There was a problem hiding this comment.
Severity: warning
The path /settings/users/edit/${viewerId} is hardcoded. It would be better for maintainability to use a named route or a constant.
Suggestions:
- Use a named route or centralize path definitions.
| </Link> | ||
| )} | ||
| {canManageCurrentOrg && settingsTab && ( | ||
| <Link to={`/settings?subtab=${settingsTab}#auto-approve`} className={actionClass} title="Open this stage in the org policy settings"> |
There was a problem hiding this comment.
Severity: warning
The path /settings?subtab=${settingsTab}#auto-approve is hardcoded. It would be better for maintainability to use a named route or a constant.
Suggestions:
- Use a named route or centralize path definitions.
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Eligibility) · Review #13487
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ❌ | author level 1 < required 2 |
| Static rules | ⏭️ | skipped (earlier stage blocked) |
| Risk score | ⏭️ | skipped (earlier stage blocked) |
| LLM classification | ⏭️ | skipped (earlier stage blocked) |
| Final decision | ❌ | eligibility: author level 1 < required 2 |
Implement User Time Zone Localization and PR Description StructureOverviewThis change introduces user-specific time zone preferences. It localizes timestamp displays across the application. It also refactors Pull Request description content into distinct sections. Technical Highlights
Impact
|
| return err | ||
| } | ||
| if next := upsertPRSection(body, section); next != body { | ||
| if next := update(body); next != body { |
There was a problem hiding this comment.
Severity: warning
This is a read-modify-write operation on the PR body. Concurrent updates could overwrite changes. Consider implementing optimistic locking or job serialization to handle this.
Suggestions:
- Implement optimistic locking using SCM API's ETag/versioning if available.
- Ensure job queue processing for a given PR is strictly serialized to prevent concurrent updates.
| } | ||
| }, [dispatch, accessToken]); | ||
|
|
||
| useSyncTimeZone(); |
There was a problem hiding this comment.
Severity: warning
A new hook is being called. Please ensure that there are no blocking operations or critical errors introduced by this hook.
Suggestions:
- Verify
useSyncTimeZoneis non-blocking and handles its own errors gracefully, as it's called at the top level.
|
|
||
| const verdictIcon = (v: string) => | ||
| v === 'pass' || v === 'approved' ? '✅' : v === 'fail' || v === 'blocked' ? '❌' : '⏭️'; | ||
| const VerdictIcon: React.FC<{ verdict: string }> = ({ verdict }) => { |
There was a problem hiding this comment.
Severity: info
This is a new component. Please ensure that the accessibility attributes, specifically aria-label, are correctly implemented.
Suggestions:
- Confirm
role='img'andaria-labelare sufficient for screen readers. Consider addingtitleattributes for hover text.
| 'inline-flex items-center gap-1.5 rounded-md border border-slate-700 px-2 py-1 text-xs text-slate-300 ' + | ||
| 'hover:border-slate-500 hover:text-white focus-visible:outline focus-visible:outline-2 focus-visible:outline-blue-400 transition-colors'; | ||
|
|
||
| const GearIcon = () => ( |
There was a problem hiding this comment.
Severity: info
This is a new component. Please ensure that the accessibility attributes, specifically aria-hidden, are correctly implemented.
Suggestions:
- Ensure
aria-hidden='true'is appropriate for purely decorative icons. If the icon conveys meaning without text, a screen reader accessible alternative might be needed.
| </svg> | ||
| ); | ||
|
|
||
| const UserIcon = () => ( |
There was a problem hiding this comment.
Severity: info
This is a new component. Please ensure that the accessibility attributes, specifically aria-hidden, are correctly implemented.
Suggestions:
- Ensure
aria-hidden='true'is appropriate for purely decorative icons. If the icon conveys meaning without text, a screen reader accessible alternative might be needed.
| </svg> | ||
| ); | ||
|
|
||
| const Chevron: React.FC<{ open: boolean }> = ({ open }) => ( |
There was a problem hiding this comment.
Severity: info
This is a new component. Please ensure that the accessibility attributes, specifically aria-hidden, are correctly implemented.
Suggestions:
- Ensure
aria-hidden='true'is appropriate for purely decorative icons. If the icon conveys meaning without text, a screen reader accessible alternative might be needed.
| className={`grid grid-cols-[1.25rem_1fr] items-center gap-x-3 gap-y-2 text-sm md:grid-cols-[1.25rem_11rem_1fr_auto] ${canExpand ? 'cursor-pointer' : ''}`} | ||
| onClick={(e) => { | ||
| // Links, the Details button and the info icon handle their own clicks. | ||
| if (canExpand && !(e.target as HTMLElement).closest('a,button,[aria-label="Help"]')) toggle(); |
There was a problem hiding this comment.
Severity: warning
This click handler appears to be complex. Please ensure that all interactive elements are correctly excluded from its scope.
Suggestions:
- Test thoroughly to ensure
e.target.closest('a,button,[aria-label="Help"]')correctly identifies all interactive elements that should prevent the parent div's click handler from firing. - Consider using
e.stopPropagation()on the interactive elements themselves if the list of selectors becomes too complex.
| <span className="hidden w-[5.5rem] md:block" aria-hidden="true" /> | ||
| )} | ||
| {canManageCurrentOrg && st.stage === 'eligibility' && viewerId && ( | ||
| <Link to={`/settings/users/edit/${viewerId}`} className={actionClass} title="Open your user settings (review level)"> |
There was a problem hiding this comment.
Severity: warning
The path /settings/users/edit/${viewerId} is hardcoded. This could lead to broken links if the path structure changes.
Suggestions:
- Use a named route or a centralized route helper to construct this path for better maintainability and to prevent broken links if the route structure changes.
| <UserIcon /> Your level | ||
| </Link> | ||
| )} | ||
| {canManageCurrentOrg && settingsTab && ( |
There was a problem hiding this comment.
Severity: warning
The path /settings?subtab=${settingsTab}#auto-approve is hardcoded. This could lead to broken links if the path structure changes.
Suggestions:
- Use a named route or a centralized route helper to construct this path for better maintainability and to prevent broken links if the route structure changes.
| const userId = useAppSelector((state) => state.Auth.user?.id); | ||
| useEffect(() => { | ||
| if (!userId) return; | ||
| const key = `lr_tz_synced:${userId}`; |
There was a problem hiding this comment.
Severity: info
The format for the localStorage key is being used. Please ensure that the keys are unique and clearly indicate their purpose.
Suggestions:
- The key
lr_tz_synced:${userId}is good for uniqueness. Ensurelrprefix is consistent with other app-specific localStorage keys.
| let zone = ''; | ||
| try { | ||
| zone = Intl.DateTimeFormat().resolvedOptions().timeZone; | ||
| if (!zone || localStorage.getItem(key) === zone) return; |
There was a problem hiding this comment.
Severity: info
Please check if localStorage.getItem(key) === zone is being used. This check can prevent redundant API calls.
Suggestions:
- Good use of
localStorageto avoid unnecessary API calls when the time zone hasn't changed.
| } | ||
| saveTimeZone(zone) | ||
| .then(() => { | ||
| try { localStorage.setItem(key, zone); } catch { /* storage blocked: retry next visit */ } |
There was a problem hiding this comment.
Severity: info
Error handling for localStorage writes is implemented. This is good practice, especially for privacy or security settings.
Suggestions:
- Catching
localStorageerrors is good practice, as it can be blocked by browser settings or security policies.
| .then(() => { | ||
| try { localStorage.setItem(key, zone); } catch { /* storage blocked: retry next visit */ } | ||
| }) | ||
| .catch(() => { /* not critical: the PR table falls back to UTC */ }); |
There was a problem hiding this comment.
Severity: info
Error handling for API calls is implemented. This is beneficial for non-critical background tasks.
Suggestions:
- Explicitly catching and ignoring errors for a non-critical background task like time zone sync is appropriate, with a fallback to UTC.
| const [overrideDraft, setOverrideDraft] = useState<Record<string, number>>({}); | ||
| const saveTimers = useRef<Record<string, ReturnType<typeof setTimeout>>>({}); | ||
| const [outcomes, setOutcomes] = useState<OutcomeRow[]>([]); | ||
| const [subtab, setSubtab] = useState('general'); |
There was a problem hiding this comment.
Severity: warning
The initial state is being derived from the URL. Please ensure that input validation is performed for the subtab parameter.
Suggestions:
- Add validation for the
subtabvalue retrieved fromwindow.location.searchto ensure it's one of the expected tab names. This prevents potential issues if an invalid or malicious value is injected into the URL.
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Eligibility) · Review #13489
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ❌ | author level 1 < required 2 |
| Static rules | ⏭️ | skipped (earlier stage blocked) |
| Risk score | ⏭️ | skipped (earlier stage blocked) |
| LLM classification | ⏭️ | skipped (earlier stage blocked) |
| Final decision | ❌ | eligibility: author level 1 < required 2 |
…ew time in local zone Auto Approve panel: aligned rows, status icons, Configure and Your level links, click a row to open its details. PR description: controls block at the top, decisions table at the bottom with a Time column in the review starter's saved time zone (new users.time_zone, saved once from the browser). LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
…ore info icon clicks A missing end marker no longer deletes the text after the start marker. The time zone is capped at 64 characters. Clicking the info icon in a stage row no longer opens the row. LiveReview Pre-Commit Check: vouched (iter:1, coverage:0%)
1ad1271 to
adae38a
Compare
What
Three related changes around auto-approve.
1. Auto Approve tab (review page)
#22C55Edirectly becausetailwind.config.jsdefinesgreenas one flat colour, sobg-green-500generates no CSS./settings?subtab=<tab>#auto-approve). The Eligibility row also has Your level, which opens the viewer's own user settings.2. PR/MR description layout
---line.3. Review time in the decisions table
users.time_zonecolumn, plusPUT /users/time-zone(session only, validates the IANA name, writes only when the value changed).localStorage).Migration
db/migrations/20261029120000_add_user_time_zone.sqladdsusers.time_zone. It must run before the new PR table query works.Docs
Updated
routes_guide/reviews/review-detail.md,routes_guide/reviews/pr-review-link.mdanddocs/pr-review-links-plan.md.Checked
npx tsc --noEmitpasses.go vetpasses oninternal/jobqueueandinternal/api/users.internal/apiwas not vetted.Not checked
🔍 LiveReview
Auto-approval decisions
⚙️ Customize risk-based approval policy for this repository or your organization.