🐛 fix(v1.6): remediate comprehensive review findings#574
Conversation
…#566 The display-honesty batch changed two surfaces these specs asserted: - #406: the one-clock maturity panel collapsed the countdown into a single '{countdown} · unlocks {date}' line, replacing 'Lifts at'. - #498: insight-only cards read 'Current' (pinned left the Update column) and the insight-kind badge was retired; pinned-ness is the persistent pin glyph on the Tag cell, asserted in both table and cards. Fixture now sets tagPinGated, the backend gate verdict the glyph keys on. Verified locally: 4 passed against the QA env.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds demo contract tests and CI build gates, converts release-cut to a recoverable digest-validated pipeline, introduces typed preview actions and update-eligibility schemas, hardens SSE and digest-cache lifecycle handling, updates candidate identity detection, clarifies authentication and support documentation, and prevents stale asynchronous preview results in the UI. Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/agent/AgentClient.ts (1)
881-897: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard queued SSE parsing before it mutates state.
A
dataevent can pass Line 882, enqueue work, thenstop()clears the stream before the queued callback runs. That callback still parses and dispatches stale events after shutdown.Proposed fix
sseProcessing = sseProcessing .then(async () => { + if (this.stopped || this.activeSseStream !== stream) { + return; + } buffer += decodedChunk; buffer = await this.processSseBuffer(buffer); })Update the stop test to emit
dataimmediately beforestop()and asserthandleEventremains untouched.🤖 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 `@app/agent/AgentClient.ts` around lines 881 - 897, Guard the queued callback in the SSE processing chain, not only the stream data handler: before appending to buffer or calling processSseBuffer, verify the client is not stopped and activeSseStream is still stream. Update the stop test to emit data immediately before stop() and assert handleEvent is not called.
🧹 Nitpick comments (1)
.github/workflows/release-cut.yml (1)
394-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRegistry list duplicated across 5 sites — extract to job-level
envto prevent divergence.
docker.io/codeswhat/drydockandquay.io/codeswhat/drydockare repeated inmeta(359-362),staging_meta(377-380),candidate_refs(394-398),source_candidates(448-452), andimage_refs(504-506). If a registry is added tometabut missed here, its GA digest silently goes unvalidated. Hoist the repo names into job-levelenvand reference them everywhere.+ env: + DOCKERHUB_REPO: docker.io/codeswhat/drydock + QUAY_REPO: quay.io/codeswhat/drydockThen use
"${DOCKERHUB_REPO}:${CANDIDATE_TAG#v}"/"${QUAY_REPO}:${CANDIDATE_TAG#v}"in the bash arrays.🤖 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 @.github/workflows/release-cut.yml around lines 394 - 398, Move the shared Docker Hub and Quay repository names into job-level env variables, then update the meta, staging_meta, candidate_refs, source_candidates, and image_refs definitions to reference those variables consistently. In the candidate_refs bash array, use DOCKERHUB_REPO and QUAY_REPO with CANDIDATE_TAG while preserving the existing GHCR entry.
🤖 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 `@apps/demo/src/mocks/handlers/notifications.ts`:
- Around line 124-132: Add shared JSON-record validation for the handlers in
apps/demo/src/mocks/handlers/notifications.ts (lines 124-132 and 95-121) and
apps/demo/src/mocks/handlers/settings.ts (lines 8-20), returning 400 for parse
failures, null, arrays, or other non-object bodies before reading fields. In the
notification PATCH handler, validate and construct an update containing only
allowed NotificationRuleUpdate fields, then have Object.assign use that update
rather than the raw body so id and arbitrary properties cannot be changed.
In `@e2e/playwright/v16-modes-pins.spec.ts`:
- Line 176: Update the ContainerFixture interface to declare the optional
tagPinGated?: boolean property, so the assignment in the fixture setup is
type-safe.
In `@scripts/release-docs-identity.test.mjs`:
- Around line 349-351: Update both authentication assertions around the visible
doesNotMatch checks to use one shared regex that matches broad-401 wording with
all or every, singular or plural call/request, and either “return `401`” or “are
rejected with `401`” phrasing. Reuse the same pattern in both assertion
locations instead of maintaining separate coverage.
- Around line 411-418: Update the generic latest-row assertion in the security
policy test to match the `latest` token case-insensitively while preserving
flexible whitespace around it, so variants such as `| Latest |` are rejected.
Keep the existing assertions in the `security policy distinguishes stable and
prerelease support` test unchanged.
In `@ui/tests/views/NotificationsView.spec.ts`:
- Around line 366-374: Update the trigger transition test around the preview
button and triggerSelect interactions so the initial draft uses Discord, then
change the selection to Slack before asserting the preview button is enabled.
Ensure the first preview request is started with trigger:discord-alerts and the
subsequent setValue switches to trigger:slack-alerts, preserving the existing
assertions and click flow.
---
Outside diff comments:
In `@app/agent/AgentClient.ts`:
- Around line 881-897: Guard the queued callback in the SSE processing chain,
not only the stream data handler: before appending to buffer or calling
processSseBuffer, verify the client is not stopped and activeSseStream is still
stream. Update the stop test to emit data immediately before stop() and assert
handleEvent is not called.
---
Nitpick comments:
In @.github/workflows/release-cut.yml:
- Around line 394-398: Move the shared Docker Hub and Quay repository names into
job-level env variables, then update the meta, staging_meta, candidate_refs,
source_candidates, and image_refs definitions to reference those variables
consistently. In the candidate_refs bash array, use DOCKERHUB_REPO and QUAY_REPO
with CANDIDATE_TAG while preserving the existing GHCR entry.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0907b193-719d-4015-afb4-f60e99948578
⛔ Files ignored due to path filters (2)
CHANGELOG.mdis excluded by!CHANGELOG.mdapps/demo/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (39)
.github/tests/ci-verify-workflow.test.ts.github/tests/release-cut-retry-workflow.test.ts.github/workflows/ci-verify.yml.github/workflows/release-cut.ymlDEPRECATIONS.mdREADME.mdSECURITY.mdapp/agent/AgentClient.test.tsapp/agent/AgentClient.tsapp/api/openapi.test.tsapp/api/openapi/schemas.test.tsapp/api/openapi/schemas.tsapp/api/preview-errors.test.tsapp/api/preview-errors.tsapp/api/preview.test.tsapp/model/container.tsapp/registries/BaseRegistry.test.tsapp/registries/BaseRegistry.tsapp/store/container.test.tsapp/watchers/providers/docker/Docker.tsapp/watchers/providers/docker/digest-cache-lifecycle.test.tsapp/watchers/providers/docker/digest-cache-lifecycle.tsapps/demo/package.jsonapps/demo/src/mocks/data/notifications.tsapps/demo/src/mocks/handlers.contract.test.tsapps/demo/src/mocks/handlers/notifications.tsapps/demo/src/mocks/handlers/settings.tscontent/docs/current/configuration/authentications/index.mdxe2e/playwright/v16-modes-pins.spec.tsscripts/release-docs-identity.test.mjsui/src/components/DataTableColumnPicker.vueui/src/locales/en/containerComponents.jsonui/src/services/preview.tsui/src/views/NotificationsView.vueui/src/views/containers/useContainerPreview.tsui/tests/components/DataTableColumnPicker.spec.tsui/tests/services/preview.spec.tsui/tests/views/NotificationsView.spec.tsui/tests/views/containers/useContainerActions.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/demo/src/mocks/handlers.contract.test.ts (1)
43-57: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify the sanitized rule is not persisted.
This assertion checks only the PATCH response. Read the rule back through the GET endpoint and assert that
arbitraryand the rewrittenidare absent there too; otherwise a handler could sanitize the response while retaining invalid fields in mock state.🤖 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 `@apps/demo/src/mocks/handlers.contract.test.ts` around lines 43 - 57, Extend the notification update test around the PATCH request and response to fetch the updated rule through the GET endpoint, then assert the persisted result does not contain the rewritten id or arbitrary field. Keep the existing response assertions and use the GET result to verify mock state was sanitized rather than only the returned payload..github/tests/release-cut-retry-workflow.test.ts (1)
147-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject hardcoded registry duplicates.
These assertions prove that environment-backed references exist, but not that hardcoded
docker.io/...orquay.io/...references were removed. A duplicate can therefore pass this test while violating the “once at job scope” contract. Add negative assertions overloadReleaseSteps()or assert the exact reference fields.Proposed test addition
expect(releaseJob?.env).toMatchObject({ DOCKERHUB_REPO: 'docker.io/codeswhat/drydock', QUAY_REPO: 'quay.io/codeswhat/drydock', }); + const releaseStepText = JSON.stringify(loadReleaseSteps()); + expect(releaseStepText).not.toContain('docker.io/codeswhat/drydock'); + expect(releaseStepText).not.toContain('quay.io/codeswhat/drydock');🤖 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 @.github/tests/release-cut-retry-workflow.test.ts around lines 147 - 177, The release registry test only verifies environment-backed references and must also reject duplicate hardcoded registry URLs. Extend the test around loadReleaseSteps and the existing releaseJob assertions to assert that step commands and relevant fields do not contain hardcoded docker.io/codeswhat/drydock or quay.io/codeswhat/drydock references, while preserving the job-scope env and env-variable usage checks.
🤖 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.
Nitpick comments:
In @.github/tests/release-cut-retry-workflow.test.ts:
- Around line 147-177: The release registry test only verifies
environment-backed references and must also reject duplicate hardcoded registry
URLs. Extend the test around loadReleaseSteps and the existing releaseJob
assertions to assert that step commands and relevant fields do not contain
hardcoded docker.io/codeswhat/drydock or quay.io/codeswhat/drydock references,
while preserving the job-scope env and env-variable usage checks.
In `@apps/demo/src/mocks/handlers.contract.test.ts`:
- Around line 43-57: Extend the notification update test around the PATCH
request and response to fetch the updated rule through the GET endpoint, then
assert the persisted result does not contain the rewritten id or arbitrary
field. Keep the existing response assertions and use the GET result to verify
mock state was sanitized rather than only the returned payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b1d533e-8dbb-48a4-9542-19fcf5aeaeb7
📒 Files selected for processing (10)
.github/tests/release-cut-retry-workflow.test.ts.github/workflows/release-cut.ymlapp/agent/AgentClient.test.tsapp/agent/AgentClient.tsapps/demo/src/mocks/handlers.contract.test.tsapps/demo/src/mocks/handlers/json.tsapps/demo/src/mocks/handlers/notifications.tsapps/demo/src/mocks/handlers/settings.tse2e/playwright/v16-modes-pins.spec.tsscripts/release-docs-identity.test.mjs
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/demo/src/mocks/handlers/settings.ts
- scripts/release-docs-identity.test.mjs
- e2e/playwright/v16-modes-pins.spec.ts
- apps/demo/src/mocks/handlers/notifications.ts
- app/agent/AgentClient.test.ts
- app/agent/AgentClient.ts
- .github/workflows/release-cut.yml
1 participant
Summary
460fb91a), so this PR supersedes ✅ test(e2e): align pinned-display and maturity-clock expectations with #566 #573Once merged, the existing
dev/v1.6→mainrelease PR #572 will automatically update to this remediated tree.Verification
Release handling
This is the complete remediation path for v1.6. Do not merge #572 until this PR is integrated and its updated candidate has completed the required exact-SHA acceptance window.
Changelog
UpdateBlocker,UpdateEligibility) and exposedContainerResource.tagPinGated+updateEligibility.{ label, href }to typed{ code, href }(API, OpenAPI schemas, UI normalization, i18n presentation).401;/health:503; discovery/status remains public).suggestedTag, gatecreatedidentity changes to legacy (no-digest) cases, and apply watcher-scoped identity keys.codecontract.action.labeltoaction.code(open-registry-settings/open-trigger-settings).Concerns
action.labeltoaction.code(including any non-covered UI paths).release-cutdispatch inputs (candidate_tag,candidate_digest) and “exact seven-day-old prerelease RC candidate” logic match the intended release policy end-to-end.