feat: implement deferred asset and image optimization items - #1313
feat: implement deferred asset and image optimization items#1313BigSimmo wants to merge 4 commits into
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdds public image optimization tooling, persisted WebP placeholders, responsive signed-image transforms, icon preloading, and non-blocking upload audit logging. ChangesImage delivery pipeline
Upload reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant document_images
participant SignedImage
participant SignedURLAPI
participant SupabaseStorage
Worker->>document_images: store placeholder_base64
SignedImage->>SignedURLAPI: request signed URL with w=400
SignedURLAPI->>SupabaseStorage: create transformed signed URL
SupabaseStorage-->>SignedURLAPI: return signed URL
SignedURLAPI-->>SignedImage: provide transformed image URL
document_images-->>SignedImage: provide placeholder data
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@phone-mockup-dev.err`:
- Around line 1-8: Remove the captured development-server log artifact from the
change, including its local paths, PID, and taskkill command; do not modify the
mockup implementation.
In `@scripts/optimize-public-images.mjs`:
- Around line 43-53: Update the image-processing loop around the per-file
try/catch to collect conversion failures instead of swallowing them, while
retaining the existing error logging. After processing all files, throw an error
if any failures were recorded, before printing the “Successfully generated
variants” message; only report success when every WebP and AVIF variant was
generated.
In `@src/app/api/images/`[id]/signed-url/route.ts:
- Around line 65-73: Validate widthParam before constructing transform in the
signed URL route: require a complete positive integer value, reject malformed or
non-integer inputs instead of relying on parseInt, and enforce a maximum width
of 400. Preserve transform as undefined when w is absent, and return the route’s
established client-error response for invalid values before calling
createSignedUrl.
In `@src/app/mockups/phone-inpage-navigation/page.tsx`:
- Around line 115-121: Replace the no-op onClick handler on the “Open clinical
record” button with the prescribed disabled-placeholder pattern, unless an
existing destination and navigation flow are available; do not leave the button
appearing actionable without behavior. Update the button in the phone in-page
navigation mockup while preserving its existing styling and label.
- Around line 76-121: Update ClinicalContent to render content based on the
active SectionLabel rather than always showing the same “Why matched”, “Safety
first”, and “Best fit” card. Use section-specific data or anchored sections for
each selectable label, and synchronize the active state with the rendered
content and scroll position while preserving the existing layout and CTA.
- Around line 296-319: Update the central summary button around the modulo
setIndex transition so it no longer wraps from the final section to the first;
make it non-interactive or reuse the bounded transition behavior of the visible
Next control, while preserving the existing summary display.
- Around line 44-70: Update the three header buttons in the page component so
none remains a misleading no-op: implement meaningful menu, therapy-selector,
and clinical-note actions where available, or mark unavailable controls with the
established disabled/aria-disabled coming-soon pattern, including a title and
sr-only explanation. Preserve each button’s existing accessible label and visual
styling while ensuring every rendered button either performs an action or
clearly communicates that it is unavailable.
In `@src/components/clinical-dashboard/signed-image.tsx`:
- Around line 56-69: Compute resolvedEndpoint before initializing shouldLoad,
then use resolvedEndpoint with getCachedSignedUrl for cache gating. Ensure
useSignedImageUrl and the initial loading decision use the same resolved cache
key, including thumbnail query parameters.
- Around line 56-69: Update the resolvedEndpoint construction in the
signed-image component to parse the endpoint query parameters and use
URLSearchParams.set("w", "400") when thumbnail is enabled, replacing any
existing width value instead of appending a duplicate. Preserve the original
endpoint when thumbnail is disabled and retain all other query parameters.
🪄 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
Run ID: 4492a342-ccb6-4d47-823f-3b5d75e5cc66
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonphone-mockup-dev.logis excluded by!**/*.log
📒 Files selected for processing (8)
package.jsonphone-mockup-dev.errscripts/optimize-public-images.mjssrc/app/api/images/[id]/signed-url/route.tssrc/app/layout.tsxsrc/app/mockups/phone-inpage-navigation/page.tsxsrc/components/clinical-dashboard/signed-image.tsxworker/main.ts
| ⨯ Another next dev server is already running. | ||
| - Local: http://localhost:4500 | ||
| - PID: 104364 | ||
| - Dir: C:\Dev\Apps\Database | ||
| - Log: .next\dev\logs\next-development.log | ||
| Run taskkill /PID 104364 /F to stop it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the captured development-server log.
This generated artifact exposes local machine details and a stale PID command without supporting the mockup. Delete it from the change.
🤖 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 `@phone-mockup-dev.err` around lines 1 - 8, Remove the captured
development-server log artifact from the change, including its local paths, PID,
and taskkill command; do not modify the mockup implementation.
| try { | ||
| await sharp(file).webp({ quality: 80 }).toFile(webpPath); | ||
| await sharp(file).avif({ quality: 80 }).toFile(avifPath); | ||
| totalFiles++; | ||
| console.log(`Optimized: ${file}`); | ||
| } catch (err) { | ||
| console.error(`Failed to optimize ${file}:`, err); | ||
| } | ||
| } | ||
| } | ||
| console.log(`Successfully generated variants for ${totalFiles} images.`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail the command when any variant cannot be generated.
A conversion failure is logged but swallowed, so CI exits 0 and reports success while variants may be missing or stale. Aggregate failures and throw before the success message.
Proposed fix
async function optimizeImages() {
console.log("Optimizing public images to WebP and AVIF...");
-+
let totalFiles = 0;
+ let failedFiles = 0;
for (const dir of DIRECTORIES) {
const pngFiles = await walkDir(dir);
-+
for (const file of pngFiles) {
@@
} catch (err) {
+ failedFiles++;
console.error(`Failed to optimize ${file}:`, err);
}
}
}
+ if (failedFiles > 0) {+ throw new Error(`Failed to generate variants for ${failedFiles} image(s).`);+ }
console.log(`Successfully generated variants for ${totalFiles} images.`);
}📝 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.
| try{ | |
| awaitsharp(file).webp({quality: 80}).toFile(webpPath); | |
| awaitsharp(file).avif({quality: 80}).toFile(avifPath); | |
| totalFiles++; | |
| console.log(`Optimized: ${file}`); | |
| }catch(err){ | |
| console.error(`Failed to optimize ${file}:`,err); | |
| } | |
| } | |
| } | |
| console.log(`Successfully generated variants for ${totalFiles} images.`); | |
| asyncfunctionoptimizeImages(){ | |
| console.log("Optimizing public images to WebP and AVIF..."); | |
| lettotalFiles=0; | |
| letfailedFiles=0; | |
| for(constdirofDIRECTORIES){ | |
| constpngFiles=awaitwalkDir(dir); | |
| for(constfileofpngFiles){ | |
| try{ | |
| awaitsharp(file).webp({quality: 80}).toFile(webpPath); | |
| awaitsharp(file).avif({quality: 80}).toFile(avifPath); | |
| totalFiles++; | |
| console.log(`Optimized: ${file}`); | |
| }catch(err){ | |
| failedFiles++; | |
| console.error(`Failed to optimize ${file}:`,err); | |
| } | |
| } | |
| } | |
| if(failedFiles>0){ | |
| thrownewError(`Failed to generate variants for ${failedFiles} image(s).`); | |
| } | |
| console.log(`Successfully generated variants for ${totalFiles} images.`); | |
| } |
🤖 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 `@scripts/optimize-public-images.mjs` around lines 43 - 53, Update the
image-processing loop around the per-file try/catch to collect conversion
failures instead of swallowing them, while retaining the existing error logging.
After processing all files, throw an error if any failures were recorded, before
printing the “Successfully generated variants” message; only report success when
every WebP and AVIF variant was generated.
| const { searchParams } = new URL(request.url); | ||
| const widthParam = searchParams.get("w"); | ||
| const transform = widthParam | ||
| ? { width: parseInt(widthParam, 10), resize: "contain" as const } | ||
| : undefined; | ||
| const signed = await supabase.storage | ||
| .from(env.SUPABASE_IMAGE_BUCKET) | ||
| .createSignedUrl(image.storage_path, signedUrlTtlSeconds); | ||
| .createSignedUrl(image.storage_path, signedUrlTtlSeconds, { transform }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
file="src/app/api/images/[id]/signed-url/route.ts"echo"== file info =="
wc -l "$file"echoecho"== outline =="
ast-grep outline "$file" --view expanded ||trueechoecho"== relevant lines =="
sed -n '1,220p'"$file"| cat -nRepository: BigSimmo/Database
Length of output: 4634
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== search for this signed-url route and w parameter =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build''signed-url|[?&]w=|searchParams.get\("w"\)|createSignedUrl\(' src .||trueRepository: BigSimmo/Database
Length of output: 17393
🏁 Script executed:
#!/bin/bashset -euo pipefail
file="src/components/clinical-dashboard/signed-image.tsx"echo"== outline =="
ast-grep outline "$file" --view expanded ||trueechoecho"== relevant lines =="
sed -n '1,240p'"$file"| cat -nRepository: BigSimmo/Database
Length of output: 10369
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== tests or docs mentioning thumbnail width =="
rg -n 'w=400|thumbnail|optimized thumbnail|createSignedUrl\(.*transform|resize: "contain"' \
src tests docs | sed -n '1,200p'Repository: BigSimmo/Database
Length of output: 1070
🌐 Web query:
Supabase storage createSignedUrl transform width maximum image resize contain invalid width NaN behavior docs
💡 Result:
When using the createSignedUrl method in Supabase Storage with image transformations, providing an invalid width value, such as NaN, does not trigger specific client-side error handling for the NaN value itself [1][2]. Instead, the behavior depends on how the underlying transformation service interprets the input. In the Supabase Storage architecture, the transformation service (often imgproxy or internal handlers) expects valid numeric values for dimensions [3][4]. If a NaN value is passed, the request may fail to produce a valid image because the transformation parameters will be malformed [4]. Key behavioral points include: 1. Client-Side Handling: The Supabase SDKs typically pass the transformation options object to the storage API [2]. The SDK does not inherently sanitize or strip NaN values before transmission [1][5]. 2. Server-Side Impact: When the storage server receives a NaN width, it cannot perform the requested resize [4]. Depending on the specific deployment and configuration, this often results in a 4xx series error (such as a 400 Bad Request) from the storage-api or the underlying transformation engine, as the requested image cannot be rendered [4]. 3. Recommended Practice: You should ensure that any values passed to the transform object are validated as finite numbers before calling createSignedUrl. Use Number.isFinite() or similar checks to ensure width and height are valid integers. Historical context: Recent updates to the Supabase JavaScript SDK (around March/April 2026) have improved how transform objects are handled, particularly ensuring that empty transformation objects do not inadvertently cause the system to route requests to the wrong render endpoint [6][7][5]. However, these fixes do not change the requirement for valid numerical input when a transformation is actually requested. [5]
Citations:
- 1: https://supabase.com/docs/reference/javascript/file-buckets-createsignedurl
- 2: https://supabase.com/docs/guides/storage/serving/image-transformations
- 3: https://supabase.com/features/image-transformations
- 4: https://github.com/supabase/storage/blob/003d5f5d/src/storage/renderer/image.ts
- 5: fix(storage): remove client-side signed URL render endpoint normalization supabase/supabase-js#2249
- 6: fix(storage): do not rewrite signed URL to render endpoint for empty transform object supabase/supabase-js#2162
- 7: getSignedUrl behavior change breaks any existing signed URL usage with an empty
transformobject supabase/supabase-js#2159
Validate and bound w before calling createSignedUrl.parseInt accepts malformed values like 12px and abc, and this route only needs thumbnail sizes, so reject non-integers and cap the width at 400 to avoid malformed transform requests and unnecessary image-processing load.
🤖 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/app/api/images/`[id]/signed-url/route.ts around lines 65 - 73, Validate
widthParam before constructing transform in the signed URL route: require a
complete positive integer value, reject malformed or non-integer inputs instead
of relying on parseInt, and enforce a maximum width of 400. Preserve transform
as undefined when w is absent, and return the route’s established client-error
response for invalid values before calling createSignedUrl.
| <button | ||
| type="button" | ||
| aria-label="Open menu" | ||
| className="grid size-11 place-items-center rounded-full text-[#aab3b1]" | ||
| onClick={() => undefined} | ||
| > | ||
| <Menu className="size-5" /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| className="flex h-12 min-w-0 flex-1 items-center gap-3 rounded-full border border-[#2b3234] bg-[#15191a] px-3 text-left shadow-lg" | ||
| onClick={() => undefined} | ||
| > | ||
| <span className="grid size-8 place-items-center rounded-full bg-[#55d5d9] text-[#082e32]"> | ||
| <Compass className="size-4" /> | ||
| </span> | ||
| <span className="flex-1 text-[15px] font-bold text-white">Therapy</span> | ||
| <ChevronDown className="size-4 text-[#8b9694]" /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| aria-label="New clinical note" | ||
| className="grid size-11 place-items-center rounded-full border border-[#2b3234] text-[#aab3b1]" | ||
| onClick={() => undefined} | ||
| > | ||
| <Bookmark className="size-4" /> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace header no-ops with actions or disabled placeholders.
These controls invoke () => undefined, so they appear interactive but do nothing. Implement their actions, or use the required disabled/aria-disabled coming-soon pattern with a title and sr-only explanation. As per coding guidelines, “Every interactive <button> must perform an action through onClick, form submission, or navigation.”
🤖 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/app/mockups/phone-inpage-navigation/page.tsx` around lines 44 - 70,
Update the three header buttons in the page component so none remains a
misleading no-op: implement meaningful menu, therapy-selector, and clinical-note
actions where available, or mark unavailable controls with the established
disabled/aria-disabled coming-soon pattern, including a title and sr-only
explanation. Preserve each button’s existing accessible label and visual styling
while ensuring every rendered button either performs an action or clearly
communicates that it is unavailable.
Source: Coding guidelines
Uh oh!
There was an error while loading. Please reload this page.
| <button | ||
| type="button" | ||
| onClick={() => undefined} | ||
| className="flex h-12 w-full items-center justify-center gap-2 rounded-full bg-[#55d5d9] text-sm font-bold text-[#073438]" | ||
| > | ||
| Open clinical record <ChevronRight className="size-4" /> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not expose a no-op clinical-record button.
“Open clinical record” is actionable UI with no behavior. Wire it to navigation or present it as the prescribed disabled placeholder until the destination exists. As per coding guidelines, unfinished features must use the explicit disabled-placeholder pattern.
🤖 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/app/mockups/phone-inpage-navigation/page.tsx` around lines 115 - 121,
Replace the no-op onClick handler on the “Open clinical record” button with the
prescribed disabled-placeholder pattern, unless an existing destination and
navigation flow are available; do not leave the button appearing actionable
without behavior. Update the button in the phone in-page navigation mockup while
preserving its existing styling and label.
Source: Coding guidelines
Uh oh!
There was an error while loading. Please reload this page.
| /** If true, appends ?w=400 to request a smaller optimized thumbnail. */ | ||
| thumbnail?: boolean; | ||
| /** Tiny base64 Blurhash/LQIP placeholder to show while loading. */ | ||
| placeholderBase64?: string | null; | ||
| }) { | ||
| const [shouldLoad, setShouldLoad] = useState(() => Boolean(getCachedSignedUrl(endpoint))); | ||
| const [loaded, setLoaded] = useState(false); | ||
| const [lightboxOpen, setLightboxOpen] = useState(false); | ||
| const frameRef = useRef<HTMLDivElement | null>(null); | ||
| const triggerRef = useRef<HTMLButtonElement>(null); | ||
| const { url, failed, retry, markFailed } = useSignedImageUrl(endpoint, shouldLoad); | ||
| const resolvedEndpoint = thumbnail | ||
| ? endpoint.includes("?") ? `${endpoint}&w=400` : `${endpoint}?w=400` | ||
| : endpoint; | ||
| const { url, failed, retry, markFailed } = useSignedImageUrl(resolvedEndpoint, shouldLoad); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Use the resolved endpoint for cache gating.
shouldLoad checks the original endpoint, but useSignedImageUrl fetches resolvedEndpoint. With thumbnail=true, a cached full-size URL can bypass IntersectionObserver and trigger an off-screen thumbnail request, while a cached thumbnail URL is ignored. Resolve the endpoint before initializing state and probe that same cache key.
🤖 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/clinical-dashboard/signed-image.tsx` around lines 56 - 69,
Compute resolvedEndpoint before initializing shouldLoad, then use
resolvedEndpoint with getCachedSignedUrl for cache gating. Ensure
useSignedImageUrl and the initial loading decision use the same resolved cache
key, including thumbnail query parameters.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Replace an existing w parameter instead of appending another one.
If endpoint already contains w, this produces values such as ?w=800&w=400. The route uses searchParams.get("w"), so it will honor the first value and the thumbnail will not necessarily be 400px. Use URLSearchParams.set("w", "400") when constructing the endpoint.
🤖 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/clinical-dashboard/signed-image.tsx` around lines 56 - 69,
Update the resolvedEndpoint construction in the signed-image component to parse
the endpoint query parameters and use URLSearchParams.set("w", "400") when
thumbnail is enabled, replacing any existing width value instead of appending a
duplicate. Preserve the original endpoint when thumbnail is disabled and retain
all other query parameters.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:edbf329965
ℹ️ 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".
| const transform = widthParam | ||
| ? { width: parseInt(widthParam, 10), resize: "contain" as const } | ||
| : undefined; |
There was a problem hiding this comment.
When w is non-numeric, zero, negative, partially numeric, or excessively large, parseInt produces NaN or an out-of-range value that is passed directly to Supabase instead of returning a client error. Requests such as ?w=abc or ?w=-1 can therefore produce a provider error or an unusable transformed URL; validate a bounded positive integer before constructing transform, with focused route tests for malformed and boundary values.
AGENTS.md reference: AGENTS.md:L551-L559
Useful? React with 👍 / 👎.
| const signed = await supabase.storage | ||
| .from(env.SUPABASE_IMAGE_BUCKET) | ||
| .createSignedUrl(image.storage_path, signedUrlTtlSeconds); | ||
| .createSignedUrl(image.storage_path, signedUrlTtlSeconds, { transform }); |
There was a problem hiding this comment.
For every existing request without w, this now calls createSignedUrl(path, ttl, { transform: undefined }), while the unchanged ownership, public-document, and legacy-generation cases in tests/private-access-routes.test.ts assert the established two-argument call. Those focused tests will fail even though no transform was requested; conditionally omit the options argument or update the contract tests if the three-argument shape is intentional.
AGENTS.md reference: AGENTS.md:L553-L559
Useful? React with 👍 / 👎.
| const placeholderBuffer = await sharp(bytes) | ||
| .resize(16, null, { withoutEnlargement: true }) | ||
| .webp({ quality: 20 }) | ||
| .toBuffer(); | ||
| placeholderBase64 = `data:image/webp;base64,${placeholderBuffer.toString("base64")}`; |
There was a problem hiding this comment.
Every retained ingestion image is now decoded and WebP-encoded, but withImageTableMetadata deletes the metadata in src/lib/document-detail.ts:136, buildVisualEvidence does not project placeholder_base64, and no repository caller passes placeholderBase64 to SignedImage. The UI therefore always uses the existing skeleton while ingestion pays the added native CPU and database-storage cost for every image; propagate the field through the relevant contracts and call sites, or remove the eager generation until it has a consumer.
AGENTS.md reference: AGENTS.md:L551-L559
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| console.error(`Failed to optimize ${file}:`, err); | ||
| } |
There was a problem hiding this comment.
If WebP generation succeeds but AVIF generation fails—or either conversion fails outright—the per-file catch only logs the error, leaves any partial output behind, and allows npm run optimize:images to exit successfully. Automation and developers therefore cannot distinguish a complete asset set from a partial one; record failures and return a nonzero exit status after processing, with a focused test that makes one Sharp conversion reject.
Useful? React with 👍 / 👎.
| <p className="text-[10px] font-bold uppercase tracking-[0.18em] text-[#55d5d9]">{kicker}</p> | ||
| <h2 className="mt-1 text-lg font-semibold text-white">{title}</h2> | ||
| </div> | ||
| <div className="mx-auto h-[690px] w-full max-w-[390px] overflow-hidden rounded-[34px] border border-[#343b3d] bg-[#0b0e0f] shadow-[0_30px_80px_rgba(0,0,0,.45)] ring-4 ring-[#171b1c]"> |
There was a problem hiding this comment.
At phone widths, the fixed 690px frame contains the 116px header plus the navigation and clinical content, but overflow-hidden clips anything below the frame and no descendant provides a vertical scroll container. Longer wrapped content and the bottom action become unreachable, so the prototype cannot exercise the in-page navigation in realistic mobile conditions; keep the header fixed within a flex column and make the remaining frame body scrollable, then verify at a narrow phone viewport.
Useful? React with 👍 / 👎.
Resolve phone-inpage-navigation mockup add/add by taking main typography tokens. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:6780f090c3
ℹ️ 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".
| zoomable = false, | ||
| caption, | ||
| aspectRatio, | ||
| thumbnail = false, |
There was a problem hiding this comment.
Because thumbnail defaults to false and no SignedImage call site in this commit sets it, existing document and evidence previews continue requesting the unmodified endpoint and downloading full-resolution images; the new ?w=400 signing path is therefore unused by the application. Enable this for inline previews while retaining the original endpoint for the lightbox, and add a focused render test asserting that SourceImage or DocumentImage fetches a width-qualified URL.
Useful? React with 👍 / 👎.
| await sharp(file).webp({ quality: 80 }).toFile(webpPath); | ||
| await sharp(file).avif({ quality: 80 }).toFile(avifPath); |
There was a problem hiding this comment.
When npm run optimize:images is executed, these calls create sibling WebP and AVIF files, but every application reference under the targeted directories still points to the PNG files and there is no picture/source or format-selection logic. The command therefore produces unused artifacts without reducing any served image payload; update the consumers or generated paths to select these variants and add a check proving that an optimized asset is actually referenced.
Useful? React with 👍 / 👎.
BigSimmo
commented
Jul 28, 2026
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:6780f090c3
ℹ️ 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".
| const signed = await supabase.storage | ||
| .from(env.SUPABASE_IMAGE_BUCKET) | ||
| .createSignedUrl(image.storage_path, signedUrlTtlSeconds); | ||
| .createSignedUrl(image.storage_path, signedUrlTtlSeconds, { transform }); |
There was a problem hiding this comment.
For every existing request without ?w=..., this now invokes createSignedUrl(path, ttl, { transform: undefined }); the focused route tests at tests/private-access-routes.test.ts lines 1117, 1147, and 1176 assert the established two-argument call, so those tests fail before the thumbnail path is exercised. Pass the options object only when a validated width exists, or update the existing expectations alongside a new transformed-width case.
Useful? React with 👍 / 👎.
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #6250 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
f1e0693 to
a283050CompareThere was a problem hiding this comment.
Actionable comments posted: 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/app/api/upload/route.ts`:
- Around line 293-309: The document-scoped audit warning in the upload handler
is unreachable because writeAuditLog absorbs failures. Update writeAuditLog to
propagate failures after its existing logging, or move the warning into that
helper, ensuring audit failures emit a warning containing documentId while
preserving successful uploads.
🪄 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
Run ID: 0898bda1-d85c-4d8e-b9e6-68dd57f7109d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/app/api/upload/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
| try { | ||
| await writeAuditLog(supabase, { | ||
| ownerId: uploadOwnerId, | ||
| action: "document_upload", | ||
| resourceType: "document", | ||
| resourceId: documentId, | ||
| // `audit_logs` is retained indefinitely. Keep only operational facts there; | ||
| // the user-controlled filename and content hash remain on the scoped document | ||
| // record, not in the durable audit trail. | ||
| metadata: { fileType: file.type, fileSize: file.size }, | ||
| }); | ||
| } catch (auditError) { | ||
| logger.warn("Upload succeeded but audit log failed", { | ||
| documentId, | ||
| message: auditError instanceof Error ? auditError.message : String(auditError), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ensure audit failures reach this handler if the document-scoped warning is required.
writeAuditLog already catches and logs both Supabase errors and thrown exceptions in src/lib/audit.ts:62-80, then resolves successfully. Consequently, this catch is not entered for actual audit-log failures, so the promised warning containing documentId is never emitted. Either make writeAuditLog return a failure result/throw after logging, or move the document-scoped warning into that helper.
🤖 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/app/api/upload/route.ts` around lines 293 - 309, The document-scoped
audit warning in the upload handler is unreachable because writeAuditLog absorbs
failures. Update writeAuditLog to propagate failures after its existing logging,
or move the warning into that helper, ensuring audit failures emit a warning
containing documentId while preserving successful uploads.
Implements the remaining asset optimization items from the asset optimization audit: pre-generates WebP/AVIF variants, preloads SVGs, adds Blurhash/LQIP for signed images, and uses Supabase image transformations for thumbnails.