Skip to content

v2.2.4#13

Open
byteful wants to merge 6 commits into
mainfrom
dev
Open

v2.2.4#13
byteful wants to merge 6 commits into
mainfrom
dev

Conversation

@byteful

@byteful byteful commented May 21, 2026

Copy link
Copy Markdown
Member

Confidence Score: 4/5

This is close, but the alerts list parsing should be fixed before merging.

  • The new alerts page can display an empty history when the API returns the same envelope shape used by the other admin services.
  • The rest of the latest changes look contained to frontend routing, service mapping, and UI updates.

client/src/lib/services/alerts-service.ts

Important Files Changed

Filename Overview
client/src/lib/services/alerts-service.ts Adds the alert API client, but the list response parsing does not match the existing admin envelope convention.
client/src/pages/AlertsPage.tsx Adds the alert management UI that depends on the alerts service returning the correct list payload.
client/src/lib/services/system-service.ts Updates prompt APIs for the new single-prompt editor.
client/src/pages/SystemPromptsPage.tsx Replaces the multi-strictness prompt editor with a single prompt editing flow.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
client/src/lib/services/alerts-service.ts:75-79
**Unwrap alert responses**

This reads `items` from the top-level response, but the other admin services unwrap the API envelope before mapping payloads. If `/v1/admin/alerts` returns the same shape as the rest of these endpoints, such as `{ success: true, data: { items: [...] } }`, this returns an empty array and the Alerts page shows no alerts even when alerts exist.

```suggestion
  async getAlerts(): Promise<SystemAlert[]> {
    const raw = await requestJsonRaw<unknown>('/v1/admin/alerts');
    const payload = isRecord(raw) && 'data' in raw ? raw.data : raw;
    const items = Array.isArray(payload)
      ? (payload as RawSystemAlert[])
      : (isRecord(payload) && Array.isArray(payload.items) ? (payload.items as RawSystemAlert[]) : []);
    return items.map(mapAlert);
  },
```

Reviews (2): Last reviewed commit: "proto-json client compatibility" | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

  • Rule used - This is a React frontend project on React 19 with ... (source)

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented May 22, 2026

Copy link
Copy Markdown

Deploying modl-admin with  Cloudflare Pages  Cloudflare Pages

Latest commit: 834d908
Status: ✅  Deploy successful!
Preview URL: https://21142b47.admin-d0r.pages.dev
Branch Preview URL: https://dev.admin-d0r.pages.dev

View logs

@byteful byteful marked this pull request as ready for review May 22, 2026 04:29
lastActivityAt: normalizeDateValue(data.lastActivityAt),
loggedInIps: [],
isAuthenticated: true,
isAuthenticated: data.isAuthenticated ?? true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Validate login state
This returns data.isAuthenticated directly whenever the field is present. If the API returns a string value such as "false", the runtime value becomes a truthy string and callers like ProtectedRoute can treat the login as authenticated until the session refetch corrects it. Keep the mapper consistent with mapSessionPayload by only accepting a real boolean true as authenticated.

Suggested change
isAuthenticated: data.isAuthenticated ?? true,
isAuthenticated: data.isAuthenticated === true,
Prompt To Fix With AI
This is a comment left during a code review.
Path: client/src/lib/services/auth-service.ts
Line: 67

Comment:
**Validate login state**
This returns `data.isAuthenticated` directly whenever the field is present. If the API returns a string value such as `"false"`, the runtime value becomes a truthy string and callers like `ProtectedRoute` can treat the login as authenticated until the session refetch corrects it. Keep the mapper consistent with `mapSessionPayload` by only accepting a real boolean `true` as authenticated.

```suggestion
      isAuthenticated: data.isAuthenticated === true,
```

How can I resolve this? If you propose a fix, please make it concise.

},

async updateAlert(id: string, payload: AlertPayload): Promise<SystemAlert> {
const raw = await requestJsonRaw<unknown>(`/v1/admin/alerts/${id}`, {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Encode alert ids
The alert id is interpolated directly into the URL path. If an id contains path-significant characters like /, ?, #, or %, the request can be routed to the wrong endpoint or have part of the id interpreted as a query or fragment. Encode the path segment before building the update URL.

Suggested change
const raw = await requestJsonRaw<unknown>(`/v1/admin/alerts/${id}`, {
const raw = await requestJsonRaw<unknown>(`/v1/admin/alerts/${encodeURIComponent(id)}`, {
Prompt To Fix With AI
This is a comment left during a code review.
Path: client/src/lib/services/alerts-service.ts
Line: 82

Comment:
**Encode alert ids**
The alert id is interpolated directly into the URL path. If an id contains path-significant characters like `/`, `?`, `#`, or `%`, the request can be routed to the wrong endpoint or have part of the id interpreted as a query or fragment. Encode the path segment before building the update URL.

```suggestion
    const raw = await requestJsonRaw<unknown>(`/v1/admin/alerts/${encodeURIComponent(id)}`, {
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +75 to +79
async getAlerts(): Promise<SystemAlert[]> {
const raw = await requestJsonRaw<unknown>('/v1/admin/alerts');
const items = isRecord(raw) && Array.isArray(raw.items) ? (raw.items as RawSystemAlert[]) : [];
return items.map(mapAlert);
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Unwrap alert responses

This reads items from the top-level response, but the other admin services unwrap the API envelope before mapping payloads. If /v1/admin/alerts returns the same shape as the rest of these endpoints, such as { success: true, data: { items: [...] } }, this returns an empty array and the Alerts page shows no alerts even when alerts exist.

Suggested change
async getAlerts(): Promise<SystemAlert[]> {
const raw = await requestJsonRaw<unknown>('/v1/admin/alerts');
const items = isRecord(raw) && Array.isArray(raw.items) ? (raw.items as RawSystemAlert[]) : [];
return items.map(mapAlert);
},
async getAlerts(): Promise<SystemAlert[]> {
const raw = await requestJsonRaw<unknown>('/v1/admin/alerts');
const payload = isRecord(raw) && 'data' in raw ? raw.data : raw;
const items = Array.isArray(payload)
? (payload as RawSystemAlert[])
: (isRecord(payload) && Array.isArray(payload.items) ? (payload.items as RawSystemAlert[]) : []);
return items.map(mapAlert);
},

Rule Used: This is a React frontend project on React 19 with ... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: client/src/lib/services/alerts-service.ts
Line: 75-79

Comment:
**Unwrap alert responses**

This reads `items` from the top-level response, but the other admin services unwrap the API envelope before mapping payloads. If `/v1/admin/alerts` returns the same shape as the rest of these endpoints, such as `{ success: true, data: { items: [...] } }`, this returns an empty array and the Alerts page shows no alerts even when alerts exist.

```suggestion
  async getAlerts(): Promise<SystemAlert[]> {
    const raw = await requestJsonRaw<unknown>('/v1/admin/alerts');
    const payload = isRecord(raw) && 'data' in raw ? raw.data : raw;
    const items = Array.isArray(payload)
      ? (payload as RawSystemAlert[])
      : (isRecord(payload) && Array.isArray(payload.items) ? (payload.items as RawSystemAlert[]) : []);
    return items.map(mapAlert);
  },
```

**Rule Used:** This is a React frontend project on React 19 with ... ([source](https://app.greptile.com/modl-gg/-/custom-context?memory=b7532101-0c9e-4ab6-b168-353a105ba593))

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant