fix: harden anonymous quotas and metadata origins#550
Conversation
…findings # Conflicts: # docs/branch-review-ledger.md # src/lib/rag-extractive-answer.ts # tests/bounded-json-body.test.ts # tests/private-access-routes.test.ts # tests/source-backed-recovery-cross-reference.test.ts
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughSummary by CodeRabbit
WalkthroughMetadata origins now resolve per request, document navigation URLs use a shared helper, anonymous upload quotas apply a global ceiling, API identity derivation trusts deployment proxy headers, and UI smoke tests use owner-scoped session storage. ChangesRequest behavior updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RootLayout
participant RequestHeaders
participant ResolveMetadataBase
RootLayout->>RequestHeaders: read request headers
RootLayout->>ResolveMetadataBase: resolve metadata origin
ResolveMetadataBase-->>RootLayout: return metadataBase
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 7❌ Failed checks (1 warning, 6 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 3
🤖 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/app/layout.tsx`:
- Around line 30-34: Update the NEXT_PUBLIC_SITE_URL parsing in the metadata
generation flow to handle malformed non-empty values without allowing new
URL(configured) to throw unexpectedly. Catch the parse failure and fall back to
the request origin, while preserving the existing http/https validation for
successfully parsed URLs.
In `@tests/document-viewer-navigation-source.test.ts`:
- Around line 6-9: Update the regression test around documentPageHref to assert
the helper regex match succeeds instead of defaulting to an empty body. Prefer
invoking the extracted helper to verify its generated URL contains the document
route and page parameter while omitting the unrelated chunk parameter; otherwise
strengthen the source assertion to validate that contract rather than only one
exact string.
In `@tests/production-metadata-source.test.ts`:
- Around line 4-11: Replace the source-text assertions in the production
metadata test with behavior-level tests for the metadata resolver used by
generateMetadata. Cover configured HTTP and HTTPS origins, forwarded
host/protocol fallback when configuration is absent, and malformed
NEXT_PUBLIC_SITE_URL handling without throwing; extract a pure resolver if
needed and assert the resulting metadata origin.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c47e1a0-7437-4a78-889f-aedac8ce81df
📒 Files selected for processing (9)
.env.examplesrc/app/layout.tsxsrc/components/DocumentViewer.tsxsrc/lib/api-rate-limit.tssrc/lib/public-api-access.tstests/api-rate-limit-fallback.test.tstests/document-viewer-navigation-source.test.tstests/production-metadata-source.test.tstests/public-api-access.test.ts
💤 Files with no reviewable changes (1)
- src/components/DocumentViewer.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/document-viewer-navigation-source.test.ts (1)
13-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover non-finite page inputs
Add assertions for
NaNandInfinityafter normalizing them in the helper; otherwise those invalid runtime values can regress topage=NaNorpage=Infinitywithout test coverage.🤖 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 `@tests/document-viewer-navigation-source.test.ts` around lines 13 - 14, Add assertions in the “normalizes invalid page numbers to the first page” test for documentPageHref with NaN and Infinity inputs, verifying both normalize to the first-page URL with page=1.
🤖 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/lib/document-viewer-navigation.ts`:
- Around line 2-4: Update documentPageHref to normalize non-finite page values
to page 1 before applying truncation and the existing minimum-page clamp, so NaN
and Infinity produce ?page=1. Add coverage for non-finite inputs, preserving the
current behavior for finite page values.
---
Nitpick comments:
In `@tests/document-viewer-navigation-source.test.ts`:
- Around line 13-14: Add assertions in the “normalizes invalid page numbers to
the first page” test for documentPageHref with NaN and Infinity inputs,
verifying both normalize to the first-page URL with page=1.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bc5f81c9-45ea-4dc1-a5f0-368ac89223d1
📒 Files selected for processing (6)
src/app/layout.tsxsrc/components/DocumentViewer.tsxsrc/lib/document-viewer-navigation.tssrc/lib/metadata-base.tstests/document-viewer-navigation-source.test.tstests/production-metadata-source.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cd455492f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex please fix all opened comments, then mark as resolved |
|
Summary
Git / PR
Testing
|
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Note Docstrings generation - SUCCESS |
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Docstrings generation was requested by @BigSimmo. * #550 (comment) The following files were modified: * `src/app/layout.tsx` * `src/components/DocumentViewer.tsx` * `src/lib/api-rate-limit.ts` * `src/lib/document-viewer-navigation.ts` * `src/lib/metadata-base.ts` * `src/lib/public-api-access.ts`
|
✅ Created PR with unit tests: #555 |
|
✅ Created PR with unit tests: #556 |
BigSimmo
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai run pre-merge checks |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
|
@coderabbitai run pre-merge checks |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai ignore pre-merge checks |
ℹ️ No Pre-Merge checks found.No Per-Merge checks were run on this pull request. No override is required. |
Docstrings generation was requested by @BigSimmo. * #550 (comment) The following files were modified: * `src/app/layout.tsx` * `src/components/DocumentViewer.tsx` * `src/lib/api-rate-limit.ts` * `src/lib/document-viewer-navigation.ts` * `src/lib/metadata-base.ts` * `src/lib/public-api-access.ts` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 participant
Summary
mainVerification
node node_modules/vitest/vitest.mjs run tests/source-backed-recovery-cross-reference.test.ts tests/bounded-json-body.test.ts tests/document-viewer-navigation-source.test.ts tests/production-metadata-source.test.ts tests/public-api-access.test.ts tests/api-rate-limit-fallback.test.ts tests/private-access-routes.test.ts --reporter=dot: passed (7 files, 131 tests) on final source behavior.node node_modules/vitest/vitest.mjs run tests/production-metadata-source.test.ts tests/public-api-access.test.ts --reporter=dot: passed (2 files, 10 tests) after the documentation-only follow-up.node node_modules/eslint/bin/eslint.js src/app/layout.tsx src/lib/document-viewer-navigation.ts src/lib/metadata-base.ts src/lib/public-api-access.ts tests/document-viewer-navigation-source.test.ts tests/production-metadata-source.test.ts tests/public-api-access.test.ts --max-warnings 0: passed on final source behavior.npm run typecheck: passed on final source behavior.node node_modules/prettier/bin/prettier.cjs --check src/lib/document-viewer-navigation.ts src/lib/metadata-base.ts src/lib/public-api-access.ts tests/document-viewer-navigation-source.test.ts tests/production-metadata-source.test.ts tests/public-api-access.test.ts: passed on final source behavior.git diff --check: passed on final local changes.node node_modules/eslint/bin/eslint.js tests/ui-tools-collapse.spec.ts --max-warnings 0: passed after correcting the stale split-pane assertion.node node_modules/prettier/bin/prettier.cjs --check tests/ui-tools-collapse.spec.ts: passed after correcting the stale split-pane assertion.npm run verify:cheap: passed on the integrated branch (195 files passed, 1 skipped; 1,757 tests passed, 1 skipped).npm run format:check: passed on the integrated branch.npm run check:production-readiness:ci: passed (READY; provider configuration warnings only).npm run verify:pr-local; its production-build stage rejects this isolated worktree's external dependency junction.Buildrannpm run buildon heada035fa7d7: passed.npm run verify:ui;npm run ensurefailed because Turbopack rejects the isolated worktree's external dependency junction.Critical UI smokerannpm run test:e2e:criticalon heada035fa7d7: passed (8 tests). GitHub ActionsAdvisory UI regressionrannpm run test:e2e:advisoryon the same head: passed (129 tests).npm run verify:release; provider/release workflow remains confirmation-gated.Retrieval/ranking/scoring behavior is unchanged, so the live retrieval-quality gate is not applicable. The branch does not invoke provider-backed answer generation.
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Review findings resolved
mainguard and regression coverage from fix(rag): don't rescue bare cross-reference answers in source-backed recovery #538.cf-connecting-ip, uses the proxy-appended right edge ofx-forwarded-for, and retains the global upload ceiling.mainbounded reader and 413 regression coverage.mainshared JSON rate-limit response.