feat: improve admin queue diagnostics and recovery ui#571
Conversation
|
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactors the ImageVersions header into a styled card/toolbar, adds timestamp-to-age helpers and “started” build age metrics to the dashboard, and expands queue/job/build types plus diagnostics to compute age-based stale lists, maxed-out failures, and a “Likely Blockers” summary in the queue panel. ChangesVersion UI and Queue Diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/components/docs/versions/image-job-admin-actions.tsxOops! Something went wrong! :( ESLint: 10.3.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by src/components/docs/versions/queue-management-panel.tsxOops! Something went wrong! :( ESLint: 10.3.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by src/components/docs/versions/build-status-dashboard.tsxOops! Something went wrong! :( ESLint: 10.3.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Visit the preview URL for this PR (updated for commit bd011e1): https://game-ci-5559f--pr571-fix-image-row-admin-4da2rmrp.web.app (expires Fri, 22 May 2026 00:25:47 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1f0574f15f83e11bfc148eae8646486a6d0e078b |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/docs/versions/unity-version.tsx (1)
61-76:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftNested
<button>elements in row header: move admin actions outside the toggle button.
ImageJobAdminActionsrenders two<button>elements (lines 122 and 137 ofimage-job-admin-actions.tsx), placed as a direct child inside the outer<button className={styles.versionButton}>(line 63–69 ofunity-version.tsx). Nested interactive elements are invalid HTML and cause unpredictable click handling—clicking inner buttons may also trigger the row toggle or cause hydration warnings.Refactor to move
ImageJobAdminActionsout of the toggle button; render both as siblings (e.g., flex wrapper) or replace the row toggle with a non-button clickable element.🤖 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 `@src/components/docs/versions/unity-version.tsx` around lines 61 - 76, The row toggle button in unity-version.tsx currently contains the ImageJobAdminActions component (which itself renders buttons), causing nested interactive elements; move ImageJobAdminActions out of the toggle button so they are siblings. Concretely: replace the single <button className={styles.versionButton} onClick={ToggleEnable}> wrapper with a wrapper element (e.g., a flex <div> or existing row container) that contains the toggle <button> (keep ToggleEnable and non-interactive bits like id, icon, DateTime, ShowAndCopyChangeSetHashButton inside that button) and render <ImageJobAdminActions ciJobId={id} status={status} /> as a sibling element to the toggle button; adjust CSS (styles.versionButton / row wrapper) to preserve layout and ensure the toggle only surrounds non-interactive content so inner admin buttons are not nested.
🧹 Nitpick comments (1)
src/components/docs/versions/image-job-admin-actions.tsx (1)
87-99: ⚡ Quick winLog swallowed per-build errors so admins can diagnose partial failures.
The catch on line 96 discards the underlying error. When the notification reports
Reset 3/5 builds. 2 failed., there's no trail to figure out why — the admin would have to re-run with DevTools open and patch in logging. A singleconsole.errorkeeps the user-facing UX identical while preserving the failure context.♻️ Proposed fix
for (const build of builds) { try { const payload = endpoint === 'retryBuild' ? { buildId: build.buildId, relatedJobId: build.relatedJobId } : { buildId: build.buildId }; // Sequential calls avoid hammering the backend for a single image row action. await callEndpoint(endpoint, payload); succeeded += 1; - } catch { + } catch (error) { + console.error(`Admin action "${action}" failed for build ${build.buildId}:`, error); failed += 1; } }🤖 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 `@src/components/docs/versions/image-job-admin-actions.tsx` around lines 87 - 99, The catch block inside the for-loop that iterates over builds (around the try { await callEndpoint(endpoint, payload); } catch { ... }) is swallowing per-build errors; update the catch to log the thrown error and contextual info (e.g., the build object or build.buildId and endpoint) via console.error (or processLogger if available) so admins can diagnose partial failures while keeping the existing succeeded/failed counting and user-facing notification behavior intact.
🤖 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 `@src/components/docs/versions/image-job-admin-actions.tsx`:
- Around line 67-71: The code currently calls response.json() unconditionally
which throws on non-JSON error bodies; change the logic in the block surrounding
the response handling (the const body = await response.json(); and the
subsequent if (!response.ok) branch) to first read the raw text via
response.text(), then try to JSON.parse that text inside a try/catch to produce
a parsed body; when response.ok is false build the detail using
parsedBody.message / parsedBody.error if available, otherwise fall back to the
raw text and finally to `Request failed (${response.status})`, ensuring any JSON
parse errors are swallowed and the real status and text are surfaced in the
thrown Error.
- Around line 41-44: The problem is that useFirestoreCollectionData is called
unconditionally, creating a Firestore listener per row even when status !==
'failed'; fix by moving the ciBuilds construction and the
useFirestoreCollectionData<BuildRecord> hook into a new child component (e.g.,
FailedJobBuilds or ImageJobAdminBuilds) that accepts ciJobId as a prop and
renders the build UI, then in the parent component do an early return when
status !== 'failed' so the child (and thus ciBuilds/useFirestoreCollectionData)
is only mounted for failed jobs; reference ciBuilds, useFirestoreCollectionData,
BuildRecord, ciJobId, status and buildStatus when updating the code.
---
Outside diff comments:
In `@src/components/docs/versions/unity-version.tsx`:
- Around line 61-76: The row toggle button in unity-version.tsx currently
contains the ImageJobAdminActions component (which itself renders buttons),
causing nested interactive elements; move ImageJobAdminActions out of the toggle
button so they are siblings. Concretely: replace the single <button
className={styles.versionButton} onClick={ToggleEnable}> wrapper with a wrapper
element (e.g., a flex <div> or existing row container) that contains the toggle
<button> (keep ToggleEnable and non-interactive bits like id, icon, DateTime,
ShowAndCopyChangeSetHashButton inside that button) and render
<ImageJobAdminActions ciJobId={id} status={status} /> as a sibling element to
the toggle button; adjust CSS (styles.versionButton / row wrapper) to preserve
layout and ensure the toggle only surrounds non-interactive content so inner
admin buttons are not nested.
---
Nitpick comments:
In `@src/components/docs/versions/image-job-admin-actions.tsx`:
- Around line 87-99: The catch block inside the for-loop that iterates over
builds (around the try { await callEndpoint(endpoint, payload); } catch { ... })
is swallowing per-build errors; update the catch to log the thrown error and
contextual info (e.g., the build object or build.buildId and endpoint) via
console.error (or processLogger if available) so admins can diagnose partial
failures while keeping the existing succeeded/failed counting and user-facing
notification behavior intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7244d67f-088a-4fe1-a240-8d8168167125
📒 Files selected for processing (2)
src/components/docs/versions/image-job-admin-actions.tsxsrc/components/docs/versions/unity-version.tsx
| const ciBuilds = firestore.collection('ciBuilds').where('relatedJobId', '==', ciJobId); | ||
| const { status: buildStatus, data = [] } = useFirestoreCollectionData<BuildRecord>(ciBuilds); | ||
|
|
||
| if (status !== 'failed' || buildStatus === 'loading') return null; |
There was a problem hiding this comment.
Avoid subscribing to Firestore for every non-failed job row.
useFirestoreCollectionData is invoked unconditionally, so a per-row ciBuilds listener (filtered by relatedJobId) is created for every UnityVersion rendered — including jobs that are completed, scheduled, inProgress, superseded, etc. On a docs page that renders many image versions, this fans out into N live Firestore listeners just to discard the data at the status !== 'failed' gate one line below. Gate the component before the hook runs by splitting the inner logic into a separate component.
♻️ Proposed fix: gate the Firestore subscription on status
-const ImageJobAdminActions = ({ ciJobId, status }: Props) => {
- const firestore = useFirestore();
- const { data: user } = useUser();
- const notify = useNotification();
- const [runningAction, setRunningAction] = useState<'reset' | 'retry' | null>(null);
-
- const ciBuilds = firestore.collection('ciBuilds').where('relatedJobId', '==', ciJobId);
- const { status: buildStatus, data = [] } = useFirestoreCollectionData<BuildRecord>(ciBuilds);
-
- if (status !== 'failed' || buildStatus === 'loading') return null;
+const ImageJobAdminActions = ({ ciJobId, status }: Props) => {
+ if (status !== 'failed') return null;
+ return <ImageJobAdminActionsInner ciJobId={ciJobId} />;
+};
+
+const ImageJobAdminActionsInner = ({ ciJobId }: { ciJobId: string }) => {
+ const firestore = useFirestore();
+ const { data: user } = useUser();
+ const notify = useNotification();
+ const [runningAction, setRunningAction] = useState<'reset' | 'retry' | null>(null);
+
+ const ciBuilds = firestore.collection('ciBuilds').where('relatedJobId', '==', ciJobId);
+ const { status: buildStatus, data = [] } = useFirestoreCollectionData<BuildRecord>(ciBuilds);
+
+ if (buildStatus === 'loading') return null;🤖 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 `@src/components/docs/versions/image-job-admin-actions.tsx` around lines 41 -
44, The problem is that useFirestoreCollectionData is called unconditionally,
creating a Firestore listener per row even when status !== 'failed'; fix by
moving the ciBuilds construction and the useFirestoreCollectionData<BuildRecord>
hook into a new child component (e.g., FailedJobBuilds or ImageJobAdminBuilds)
that accepts ciJobId as a prop and renders the build UI, then in the parent
component do an early return when status !== 'failed' so the child (and thus
ciBuilds/useFirestoreCollectionData) is only mounted for failed jobs; reference
ciBuilds, useFirestoreCollectionData, BuildRecord, ciJobId, status and
buildStatus when updating the code.
| const body = await response.json(); | ||
| if (!response.ok) { | ||
| const detail = body.error ? `${body.message}: ${body.error}` : body.message; | ||
| throw new Error(detail || `Request failed (${response.status})`); | ||
| } |
There was a problem hiding this comment.
Parse JSON defensively so non-JSON error bodies don't mask the real failure.
response.json() runs before the response.ok check, so any non-JSON error response (gateway HTML 5xx, empty body 504, auth proxy redirect, etc.) throws SyntaxError: Unexpected token ... and the real status code/message never reaches the admin via the notification. Read the body as text first and parse it safely.
🛡️ Proposed fix
- const body = await response.json();
- if (!response.ok) {
- const detail = body.error ? `${body.message}: ${body.error}` : body.message;
- throw new Error(detail || `Request failed (${response.status})`);
- }
- return body;
+ const rawText = await response.text();
+ let body: { message?: string; error?: string; [key: string]: unknown } = {};
+ if (rawText) {
+ try {
+ body = JSON.parse(rawText);
+ } catch {
+ // Non-JSON response; fall through with empty body so we surface the status code.
+ }
+ }
+ if (!response.ok) {
+ const detail = body.error ? `${body.message}: ${body.error}` : body.message;
+ throw new Error(detail || `Request failed (${response.status})`);
+ }
+ return body;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const body = await response.json(); | |
| if (!response.ok) { | |
| const detail = body.error ? `${body.message}: ${body.error}` : body.message; | |
| throw new Error(detail || `Request failed (${response.status})`); | |
| } | |
| const rawText = await response.text(); | |
| let body: { message?: string; error?: string; [key: string]: unknown } = {}; | |
| if (rawText) { | |
| try { | |
| body = JSON.parse(rawText); | |
| } catch { | |
| // Non-JSON response; fall through with empty body so we surface the status code. | |
| } | |
| } | |
| if (!response.ok) { | |
| const detail = body.error ? `${body.message}: ${body.error}` : body.message; | |
| throw new Error(detail || `Request failed (${response.status})`); | |
| } | |
| return body; |
🤖 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 `@src/components/docs/versions/image-job-admin-actions.tsx` around lines 67 -
71, The code currently calls response.json() unconditionally which throws on
non-JSON error bodies; change the logic in the block surrounding the response
handling (the const body = await response.json(); and the subsequent if
(!response.ok) branch) to first read the raw text via response.text(), then try
to JSON.parse that text inside a try/catch to produce a parsed body; when
response.ok is false build the detail using parsedBody.message /
parsedBody.error if available, otherwise fall back to the raw text and finally
to `Request failed (${response.status})`, ensuring any JSON parse errors are
swallowed and the real status and text are surfaced in the thrown Error.
frostebite
changed the title
webbertakken
force-pushed
the
fix/image-row-admin-retry
branch
from
4625bf2 to
bd011e1
Compare
2 participants
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features
UI Improvements