-
Notifications
You must be signed in to change notification settings - Fork 0
Edge redirect function enhancements #410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughUpdates domain configuration in the linkry edge function by replacing domains with new prefixed variants and narrows slug-to-query mapping logic to single-part hostnames only. Adds comprehensive unit tests for CloudFront Lambda@Edge handler covering redirects, cache control, organization shortcodes, and error scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/linkryEdgeFunction/main.test.ts (1)
1-476: Excellent test coverage!The test suite is comprehensive and well-structured, covering:
- Empty path handling with proper redirects
- Successful DynamoDB lookups with correct query parameters
- Organization shortcode logic for all domain variants
- Fallback scenarios (missing items, errors, malformed data)
- Path normalization and edge cases
- Cache control headers across all response types
The tests properly validate the new domain structure (
.go.acm.illinois.edu,.go.aws.qa.acmuiuc.org,.acm.gg) and the updated slug-to-query mapping logic.Minor formatting issues flagged by ESLint can be addressed:
- }; + }The import extension issue (line 8) may need to be resolved based on your project's TypeScript/ESLint configuration, though it's typically a configuration matter rather than a code issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/linkryEdgeFunction/index.ts(2 hunks)tests/unit/linkryEdgeFunction/main.test.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
tests/unit/linkryEdgeFunction/main.test.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/NxCZHueWoq'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 8-8: Unexpected use of file extension "js" for "../../../src/linkryEdgeFunction/index.js"
(import/extensions)
[error] 52-52: Delete ;
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (1)
src/linkryEdgeFunction/index.ts (1)
18-22: No critical code issues found; review comment concern is about external runtime impact, not codebase stability.The BASE_DOMAINS change in
src/linkryEdgeFunction/index.tsis scoped exclusively to the linkry short link service and does not affect other domain references in the codebase (core.acm.illinois.edu, static.acm.illinois.edu, etc.). The existing codebase already uses the.go.prefix pattern for short links throughout tests and configuration files, indicating this change aligns with the current architecture.Any breaking impact would be limited to externally-issued short links using the old domain pattern—not codebase dependencies.
Summary by CodeRabbit
Release Notes
Chores
Tests