Add MAX_PAGINATION_LIMIT
#143
Conversation
- Introduced MAX_PAGINATION_LIMIT environment variable to control pagination limits. - Updated configuration schema to validate the new variable. - Modified ArticleFeedQuerySchema to utilize MAX_PAGINATION_LIMIT for dynamic limit enforcement.
WalkthroughThis pull request adds a new constant, Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/config.ts (1)
17-17: Remove debugging console.log statement.This console.log statement appears to be for debugging purposes and should be removed before merging.
-console.log(env.MAX_PAGINATION_LIMIT, typeof env.MAX_PAGINATION_LIMIT);src/articles/articles.schema.ts (1)
78-80: Consider adding type validation for MAX_PAGINATION_LIMIT.Since
env.MAX_PAGINATION_LIMITis a string being converted to a number, there's a small risk of runtime issues if the environment variable contains a non-numeric value.Consider updating the env schema in config.ts to validate it's a numeric string:
- MAX_PAGINATION_LIMIT: Type.String(), + MAX_PAGINATION_LIMIT: Type.String({ pattern: '^[0-9]+$' }),Or alternatively, add a validation check after loading the environment:
// In config.ts if (isNaN(Number(env.MAX_PAGINATION_LIMIT))) { throw new Error('MAX_PAGINATION_LIMIT must be a valid number'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env(1 hunks)src/articles/articles.schema.ts(2 hunks)src/config.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/articles/articles.schema.ts (1)
src/config.ts (1) (1)
env(16-16)
🔇 Additional comments (4)
.env (1)
9-9: LGTM: MAX_PAGINATION_LIMIT environment variable added.The new environment variable is correctly defined with a reasonable default value of 1000.
src/config.ts (1)
12-12: LGTM: MAX_PAGINATION_LIMIT added to environment schema.The new environment variable is correctly added to the configuration schema.
src/articles/articles.schema.ts (2)
1-1: LGTM: Import env from config module.The environment configuration is correctly imported to access the MAX_PAGINATION_LIMIT value.
78-80: LGTM: Pagination limit is now constrained by MAX_PAGINATION_LIMIT.The ArticleFeedQuerySchema now properly uses the configured maximum pagination limit. Converting the string to a number is appropriate here.
|
@Hajbo I tried to make the env var optional with default (1000) but it didn't work. Maybe there's a great TypeBox way to solve it but I really rather work with something I know better like ark.env or Zod. So this is kind of a quick fix where the env is forced. Let me know if that's annoying and we can hardcode it to 1000. Accepting an attempt from @coderabbitai to make optional+default work despite my experience above ^ |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ub.com/agnyz/the-bed-stack into 138-limit-does-not-have-an-upper-bound
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll look into making the 🏁 Script executed: Length of output: 4027 Let me investigate TypeBox's handling of optional values with defaults. I'll first check if there's any documentation or examples on TypeBox's GitHub repository. 🏁 Script executed: Length of output: 523 I'll investigate TypeBox further to understand how to properly implement optional environment variables with defaults. Let me look at the TypeBox documentation and check for any examples of working with environment variables in the codebase. 🌐 Web query: 💡 Result: TypeBox's Type.OptionalUsed to indicate a property may be omitted or explicitly set to Key Behaviors:
Example: const Schema = Type.Object({
name: Type.Optional(Type.String())
});
// Valid: { }, { name: "Alice" }
// Invalid with ExactOptionalPropertyTypes: { name: undefined }Type.DefaultUsed to set a default value when a property is missing, without affecting the underlying schema's required status. Key Behaviors:
Example: const Schema = Type.Object({
pageSize: Type.Default(Type.Number(), 10)
});
// If input lacks pageSize, Value.Cast(Schema, { }) → { pageSize: 10 }When to Use Each
Best Practices:
For schemas requiring both omission semantics and default initialization, combine the two: const Schema = Type.Object({
active: Type.Optional(Type.Default(Type.Boolean(), false))
});
// Input { } → Value.Cast → { active: false }Citations:
@yamcodes I've found a solution that should work for making MAX_PAGINATION_LIMIT optional with a default value. Based on TypeBox documentation, here's what you can do: // In config.ts
const envSchema = Type.Object({
// Other env variables...
MAX_PAGINATION_LIMIT: Type.Optional(Type.String()),
});
if (!Value.Check(envSchema, Bun.env)) throw new Error('Invalid env variables');
export const env = Value.Cast(envSchema, Bun.env);
// Add this after the Value.Cast to guarantee the value exists
if (env.MAX_PAGINATION_LIMIT === undefined) {
env.MAX_PAGINATION_LIMIT = "1000";
}This approach:
TypeBox's Type.Default doesn't modify the schema's required status, which is why the optional+default approach wasn't working as expected. This solution separates the schema validation from the default value application. |
- Updated MAX_PAGINATION_LIMIT in the environment schema to be a required string instead of an optional string with a default value. - This change ensures stricter validation of the pagination limit configuration.
|
No offense @coderabbitai but that's ugly. No, thanks. Also @Hajbo not really a fan of having the env config of this var be a "String" but to coalesce it to a number we probably need to use a dedicated env solution. I'm biased cause I made ark.env but we can also use T3 Env which supports standard schema and looks interesting. Just throwing it out there so we can tackle it once we finish the roadmap - it's really off topic here but just giving you context so you can help me decide what to do with this PR, now. |
|
Tip For best results, initiate chat on the files or code changes. @yamcodes No worries! I understand your concerns about the implementation style. For the current PR, you have a few practical options:
Regarding the type issues (String vs Number), that's a valid point that dedicated env solutions like ark.env or T3 Env would handle more elegantly. Those would definitely be worth exploring after completing your roadmap. What would you prefer to do with this PR now? If you're looking for minimal changes, option 2 (hardcoding to 1000) might be the most straightforward for now. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@yamcodes I completely understand your concerns about the TypeBox implementation. After reviewing your code, here are the most straightforward options for this PR:
The hardcoded approach is clean and simple until you have bandwidth to implement a proper env solution like ark.env or T3 Env after completing your roadmap. What would you prefer to do with this PR? (\ /) 🏁 Script executed: Length of output: 4009 @yamcodes Looking at your current implementation, I see the MAX_PAGINATION_LIMIT is now defined as a required string without a default. Let me check the exact changes in this PR to give you better advice on how to proceed. 🏁 Script executed: Length of output: 206 @yamcodes It seems I'm having trouble accessing the exact diff. Let me try a different approach to understand what changes are being made in this PR so I can give you proper advice. 🏁 Script executed: Length of output: 766 @yamcodes Let me examine what changes you've made to these files so I can better understand the current implementation and provide appropriate advice. 🏁 Script executed: Length of output: 1414 @yamcodes Based on the files I've examined, I now have a clear picture of what you're working with. Given your concerns about:
Here's my recommendation for this PR: Hardcode the value for now:
This approach:
Would this approach work for your current needs? (\ /) ✏️ Learnings added
|
yamcodes
changed the title
|
Tip For best results, initiate chat on the files or code changes. @yamcodes I understand your frustration with TypeBox's handling of environment variables. For this immediate PR, the hardcoded approach you mentioned seems most practical: // In src/articles/articles.schema.ts
// TODO: Replace with proper env variable using a dedicated solution in the future
const MAX_PAGINATION_LIMIT = 1000;
// Then in your schema:
limit: Type.Number({
minimum: 1,
maximum: MAX_PAGINATION_LIMIT,
default: 20
}),This gives you a clean solution now while setting the stage for proper implementation with ark.env or T3 Env after completing your roadmap. You can remove the MAX_PAGINATION_LIMIT from the envSchema in config.ts but might want to keep it in .env.example for documentation. ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@yamcodes I've looked at the current implementation and understand the challenge with TypeBox. Here's a solution that should work while keeping the codebase clean: In // Make it optional, but no default in the schema definition
MAX_PAGINATION_LIMIT: Type.Optional(Type.String()),
// Then after validation, add this code:
if (!Value.Check(envSchema, Bun.env)) {
throw new Error('Invalid env variables');
}
export const env = Value.Cast(envSchema, Bun.env);
// Add this line to set the default after validation
if (env.MAX_PAGINATION_LIMIT === undefined) {
env.MAX_PAGINATION_LIMIT = '1000';
}This approach:
Alternatively, if you'd prefer to hardcode it to 1000 as mentioned, you could remove it from env variables entirely and define it as a constant in // In articles.schema.ts
const MAX_PAGINATION_LIMIT = 1000;
// Use directly in the schema
limit: Type.Number({
minimum: 1,
maximum: MAX_PAGINATION_LIMIT,
default: 20,
}),Let me know which approach you prefer! 🏁 Script executed: Length of output: 3742 🏁 Script executed: Length of output: 595 @yamcodes I understand your frustration with TypeBox's handling of optional variables with defaults. After examining your code, I can see the issue clearly. Looking at your current implementation: // In config.ts
MAX_PAGINATION_LIMIT: Type.String(),
// In articles.schema.ts
maximum: Number(env.MAX_PAGINATION_LIMIT)You're right that TypeBox doesn't handle optional+default in an intuitive way. Based on your preferences and the constraints, here are two simple options: Option 1: Post-validation default (cleanest TypeBox approach)// In config.ts - make it optional
MAX_PAGINATION_LIMIT: Type.Optional(Type.String()),
// Then after validation:
export const env = Value.Cast(envSchema, Bun.env);
// Set default after validation
if (env.MAX_PAGINATION_LIMIT === undefined) {
env.MAX_PAGINATION_LIMIT = '1000';
}Option 2: Hardcode to 1000 (simplest approach)// In articles.schema.ts
// Remove the env import if it's only used for MAX_PAGINATION_LIMIT
const MAX_PAGINATION_LIMIT = 1000; // Hardcoded for now
// Then in the schema
maximum: MAX_PAGINATION_LIMIT,Since you mentioned you'd be open to hardcoding it to 1000, option 2 might be the quickest solution for now, especially if you plan to revisit with ark.env or T3 Env later. Which approach would you prefer? ✏️ Learnings added
|
I'm not even sure if an env var is the way do to it. To me an env var is a runtime variable that is either sensitive (database credentials), or needs to be changed easily without changing the code itself (environment name, external service urls). An upper bound for pagination sounds like a simple constant to me, if I changed it, I probably want to deploy a new version of the code. |
|
Tip For best results, initiate chat on the files or code changes. @Hajbo You make an excellent point about environment variables vs. constants. I completely agree that environment variables are best suited for:
A pagination limit doesn't really fit these criteria - it's an application-level constraint that would typically change alongside code changes with proper testing. This makes it a great candidate for a simple constant. The easiest solution would be: // In src/articles/articles.schema.ts
const MAX_PAGINATION_LIMIT = 1000;
// Then in your schema
limit: Type.Number({
minimum: 1,
maximum: MAX_PAGINATION_LIMIT,
default: 20
}),This approach is clean, type-safe, and removes all the complexity of dealing with TypeBox's environment variable handling. ✏️ Learnings added
|
Description
MAX_PAGINATION_LIMITenvironment variable to control pagination limitsCloses #138
PR Checklist
bun docsSummary by CodeRabbit