feat(website): resolve pretty URLs via CloudFront KVS, drop per-page terraform apply#1902
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a CloudFront KeyValueStore for pretty-URL route mappings, updates the CloudFront Function to read from it, adds a route-diff script, provisions supporting Terraform resources and IAM, and syncs the store during website deploys. ChangesCloudFront KVS-backed pretty-URL routing
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant AWS as AWS CLI
participant KVS as CloudFront KVS
GHA->>AWS: describe-key-value-store (ETag)
AWS-->>GHA: ETag
GHA->>AWS: list-keys → current snapshot
GHA->>GHA: routes-kvs.ts --current snapshot
GHA->>AWS: update-keys --if-match ETag
AWS-->>KVS: apply Puts and Deletes
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)
✨ 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 |
Contributor
Terraform plan —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/deploy-website.yml (1)
101-103: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winFail fast when
CF_KVS_ARNis missing in the production deploy job.At Line 102, skipping the sync step silently can leave the route map stale/empty while the deploy still reports success, reintroducing extensionless-route 404 drift.
💡 Proposed hardening
+ - name: Verify CloudFront KVS config + run: | + if [ -z "${{ vars.CF_KVS_ARN }}" ]; then + echo "CF_KVS_ARN is required for deploy-website in iii-website-prod." + exit 1 + fi + - name: Sync pretty-URL route map to CloudFront KeyValueStore - if: ${{ vars.CF_KVS_ARN != '' }} env: KVS_ARN: ${{ vars.CF_KVS_ARN }}🤖 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/deploy-website.yml around lines 101 - 103, The "Sync pretty-URL route map to CloudFront KeyValueStore" step currently silently skips when CF_KVS_ARN is missing, allowing the deploy job to report success despite this critical step not executing. Replace the conditional skip behavior with a fail-fast approach by adding a check that explicitly fails the job when CF_KVS_ARN is not set in production deployments. This ensures the route map is always synced for production or the job fails visibly, preventing stale/empty route configurations and extensionless-route 404 drift.
🤖 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 `@infra/terraform/website/cloudfront_functions/redirects.js`:
- Around line 114-118: The catch block around routes.get(uri) currently returns
notFound() for all errors, which treats both missing keys and KVS service
failures identically by returning a 404. This causes false 404s when the KVS
service fails. Modify the catch block to return the request object as-is (with
the original URI unchanged) instead of calling notFound(), allowing requests to
proceed gracefully when KVS errors occur, while still returning a proper 404
only when appropriate.
---
Nitpick comments:
In @.github/workflows/deploy-website.yml:
- Around line 101-103: The "Sync pretty-URL route map to CloudFront
KeyValueStore" step currently silently skips when CF_KVS_ARN is missing,
allowing the deploy job to report success despite this critical step not
executing. Replace the conditional skip behavior with a fail-fast approach by
adding a check that explicitly fails the job when CF_KVS_ARN is not set in
production deployments. This ensures the route map is always synced for
production or the job fails visibly, preventing stale/empty route configurations
and extensionless-route 404 drift.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 59a25a45-197b-493a-ac06-a58d195ecb18
📒 Files selected for processing (8)
.github/workflows/deploy-website.ymlinfra/terraform/website/cloudfront.tfinfra/terraform/website/cloudfront_functions/redirects.jsinfra/terraform/website/cloudfront_functions/redirects.test.jsinfra/terraform/website/iam_github_oidc.tfinfra/terraform/website/outputs.tfwebsite/scripts/routes-kvs.test.tswebsite/scripts/routes-kvs.ts
Comment on lines
+114
to
+118
| try { | ||
| request.uri = await routes.get(uri) | ||
| return request | ||
| } catch (err) { | ||
| return notFound() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For CloudFront Functions (cloudfront-js-2.0), what is the documented error shape/message from cf.kvs().get(key) for a missing key, and how should non-miss service errors be handled?
💡 Result:
In CloudFront Functions (runtime cloudfront-js-2.0), the cf.kvs().get(key) method returns a promise that rejects when a requested key is not found in the associated Key Value Store [1]. The documented guidance for handling this is to wrap the get() call in a try...catch block [1][2][3]. AWS does not explicitly document a specific error "shape" or error code property for missing keys, implying that developers should treat any rejection of the promise as an indication of a failure to retrieve the value, which typically includes cases where the key does not exist [1][3][4]. Regarding non-miss service errors (such as connectivity issues or internal KVS errors), these are also caught by the same try...catch mechanism [1][3]. Best practices for handling these scenarios include: 1. Defensive Coding: Always use a try...catch block when calling get() to prevent the function from failing execution due to an unhandled promise rejection [1][2][4]. 2. Defaulting/Graceful Degradation: If a key is missing or an error occurs, the standard pattern is to log the error for observability and proceed with a sensible default value or allow the request to proceed without the KVS-derived data [2][3][4]. 3. Avoiding Combinators: AWS recommends avoiding the use of promise combinators (like Promise.all or Promise.any) for multiple KVS lookups, as these can lead to higher memory usage and potential execution failures [1]. Instead, use sequential await calls [1].
Citations:
- 1: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/functions-custom-methods.html
- 2: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/example_cloudfront_functions_kvs_jwt_verify_section.html
- 3: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/example_cloudfront_functions_kvs_conditional_read_section.html
- 4: https://aws.amazon.com/blogs/aws/introducing-amazon-cloudfront-keyvaluestore-a-low-latency-datastore-for-cloudfront-functions/
🏁 Script executed:
# Check if there are other CloudFront Functions in the codebase using KVS or similar error handling patterns
fd '\.js$' infra/terraform/website/cloudfront_functions/ | head -20Repository: iii-hq/iii
Length of output: 175
🏁 Script executed:
# Examine the full context of the redirects.js file to understand the complete logic
wc -l infra/terraform/website/cloudfront_functions/redirects.jsRepository: iii-hq/iii
Length of output: 116
🏁 Script executed:
# Get more context around the problematic lines to understand the overall flow
sed -n '100,130p' infra/terraform/website/cloudfront_functions/redirects.jsRepository: iii-hq/iii
Length of output: 945
Differentiate KVS key-miss from KVS runtime failures to avoid false 404s.
At line 117, the catch-all catch (err) { return notFound() } treats both missing keys and KVS/data-plane service failures the same way, returning a hard 404. While a 404 is appropriate for missing keys (per the stated intent), service failures should not trigger 404 responses; instead, requests should proceed with the original URI.
AWS CloudFront Functions documentation does not document a specific error code or message shape for missing keys, making error detection unreliable. The standard approach per AWS guidance is to fail gracefully by allowing the request to proceed without KVS-derived data when any error occurs.
💡 Proposed fix
if (lastSegment.indexOf('.') === -1) {
try {
request.uri = await routes.get(uri)
return request
} catch (err) {
- return notFound()
+ // KVS errors (missing keys or service failures) should fail open,
+ // not return false 404s. Allow the request to proceed without redirection.
+ return request
}
}📝 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.
Suggested change
| try { | |
| request.uri = await routes.get(uri) | |
| return request | |
| } catch (err) { | |
| return notFound() | |
| try { | |
| request.uri = await routes.get(uri) | |
| return request | |
| } catch (err) { | |
| // KVS errors (missing keys or service failures) should fail open, | |
| // not return false 404s. Allow the request to proceed without redirection. | |
| return request | |
| } |
🤖 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 `@infra/terraform/website/cloudfront_functions/redirects.js` around lines 114 -
118, The catch block around routes.get(uri) currently returns notFound() for all
errors, which treats both missing keys and KVS service failures identically by
returning a 404. This causes false 404s when the KVS service fails. Modify the
catch block to return the request object as-is (with the original URI unchanged)
instead of calling notFound(), allowing requests to proceed gracefully when KVS
errors occur, while still returning a proper 404 only when appropriate.
…terraform apply Adding a new top-level page required editing the hardcoded htmlPretty map in the redirects CloudFront Function and running `terraform apply` to republish it — a separate, manual pipeline from the automatic content deploy. Until that apply ran, the clean URL 404'd, forcing links to the literal `.html` (MOT-3669). Move the pretty-URL -> .html map into a CloudFront KeyValueStore that deploy-website.yml syncs from website/*.html on every content deploy. The function reads the store at the edge (async). Adding a page becomes a content-only change: drop foo.html, link to /foo, merge — no terraform apply. - redirects.js: async handler; KVS lookup replaces the hardcoded map and unifies the pretty-URL rewrite with the real-404 fallback into one extensionless branch - redirects.test.js: inject a mock cf.kvs(); await handler; +6 KVS cases (46 pass) - routes-kvs.ts (+test): derive the desired map from *.html, diff vs the store - deploy-website.yml: batch update-keys step after the HTML sync (gated on CF_KVS_ARN) - terraform: aws_cloudfront_key_value_store + function association, IAM data-plane grant, routes_kvs_arn output (one-time apply; validates clean) Claude-Session: https://claude.ai/code/session_01N9Xi9vXLynC4JsaCwPJhVq
ytallo
force-pushed
the
fix/mot-3669-pretty-urls-via-kvs
branch
from
d1e7b93 to
186cc24
Compare
Contributor
Terraform plan —
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (MOT-3669)
Adding a top-level page like
/privacy-policyrequired two things on two different pipelines:privacy-policy.htmlwebsite/→ S3deploy-website.ymlaws s3 sync— automatic on push tomain/privacy-policy→/privacy-policy.htmlrewritehtmlPrettymap in the redirects CloudFront Functionterraform apply(publishes the function) — manual, no apply automationSo the
.htmlshipped automatically, but the clean-URL rewrite didn't. Until the manual apply ran,/privacy-policyfell through to the real-404 branch and 404'd — which is why the footer link had to be pointed at the literal/privacy-policy.htmlas a workaround.Fix
Move the pretty-URL →
.htmlmap out of the function source and into a CloudFront KeyValueStore (KVS). The function reads it at the edge (async);deploy-website.ymlsyncs it fromwebsite/*.htmlon every content deploy — same automatic pipeline as the HTML.Adding a page is now a content-only change: drop
foo.html, link to/foo, merge. The clean URL resolves on the next deploy. Noterraform apply.Changes
redirects.js—asynchandler; KVS lookup replaces the hardcoded map. The pretty-URL rewrite and the real-404 fallback are unified into a single extensionless-path branch, so/,/blog/*, and*.htmlnever incur a KVS read. ThenotFound()SEO behavior is preserved (KVS miss → real 404).redirects.test.js— loader strips theimport cf from 'cloudfront'line and injects a mockcf.kvs(); tests areawaited; +6 KVS cases (hit, miss→404, verbatim value, empty store, www precedence). 46/46 pass.routes-kvs.ts(+ test) — derives the desired map fromwebsite/*.html(excludesindex.html) and diffs it against the store into a batch{Puts, Deletes}. 6 unit tests.deploy-website.yml— after the HTML sync (so a key never points at a missing object), describe → list → diff →update-keys. Gated on theCF_KVS_ARNrepo var, so it no-ops until that's set.concurrency: deploy-websiteserializes deploys, keeping the describe-time ETag valid.aws_cloudfront_key_value_store.routes+ function association, a least-privilege IAM data-plane grant on the deploy role (Describe/ListKeys/UpdateKeys), and aroutes_kvs_arnoutput.terraform validatepasses. Terraform owns the store; the pipeline owns the data (the AWS provider has no key resource — data is decoupled by design).Rollout (one-time)
Terraform can't seed KVS data, so the first rollout populates the store out-of-band. To avoid a 404 window on
/manifesto+/privacy-policy, apply in two phases:key_value_store_associationsso the function keeps its current behavior).CF_KVS_ARN = terraform output -raw routes_kvs_arn, then triggerdeploy-website.yml(or run its KVS step locally) to seed/manifesto+/privacy-policy.key_value_store_associationsand apply; the function now reads a populated store. No 404 window./privacy-policy.htmlback to/privacy-policy.(If a brief 404 on those two secondary pages is acceptable, collapse to a single apply + immediate deploy.)
Test plan
redirects.test.js— 46/46 pass (40 existing behaviors unchanged + 6 KVS cases)routes-kvs.test.ts— 6/6 pass; generator verified against realwebsite/*.htmland against empty / stale-key storesterraform fmt(clean) +terraform validate(success)curl -I https://iii.dev/privacy-policy→ 200; unknown extensionless path → 404test-page.htmland confirm/test-pageresolves with no applyNote: an unrelated pre-existing test failure (
blog-posts.test.ts"finds hello-world") exists onmain— there's nohello-world.mdin the local blog content; this PR touches no blog files.Closes MOT-3669.
https://claude.ai/code/session_01N9Xi9vXLynC4JsaCwPJhVq
Summary by CodeRabbit