new api settings ui stuff#2350
Conversation
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughSeparates password reset rate limiting from general authentication by removing ChangesAPI Rate Limiting Refactor
Sequence DiagramsequenceDiagram
participant Request as Incoming Request
participant RouteServiceProvider
participant ConfigHTTP as config/http.php
participant RateLimiter
Request->>RouteServiceProvider: request to auth.post.forgot-password
RouteServiceProvider->>ConfigHTTP: load password_reset_period/password_reset
ConfigHTTP-->>RouteServiceProvider: config values
RouteServiceProvider->>RateLimiter: apply per-IP password reset limit
RateLimiter-->>Request: enforce allow/deny
Note over RouteServiceProvider: other auth routes use default limiter
Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
for clarification, this is in the admin panel -> Settings -> Misc -> API |
|
somewhat related, i can't seem to find where the auth ratelimit is used, made a discussion for that |
|
as confirmed from #2349 (reply in thread) the auth ratelimit is left over from ptero and is no longer being used due to using filament. As approved in that same discussion, i will be removing the auth ratelimit in a future commit for this PR |
|
as i do not know php nor the ratelimit system enough, i am not sure if https://github.com/O2theC/panel/blob/new-ratelimit-configs-in-ui/app/Providers/RouteServiceProvider.php#L71:L75 is as it should be. let me know if there is a certain way it should be handled or cleaned up |
|
Hi, please reopen this same PR when it's ready for review and then DM me on Discord or email me a link so I can review it, thanks! |
![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)
app/Filament/Admin/Pages/Settings.php (2)
803-964: 💤 Low valueConsider extracting the repeated fieldset pattern into a helper method.
The nine fieldsets (client, application, password_reset, websocket, backup_restore, database_create, subuser_create, file_pull, default) follow nearly identical structures: each has a label, two TextInput components for request count and period length, and a Text component for scope description. This repetition could be reduced by extracting a helper method.
💡 Example refactoring pattern
private function makeRateLimitFieldset( string $label, string $envPrefix, string $configKey, string $scopeDescription ): Fieldset { return Fieldset::make() ->label($label) ->schema([ TextInput::make("APP_API_{$envPrefix}_RATELIMIT") ->label('Requests Per Period') ->required() ->numeric() ->minValue(1) ->suffix('Requests') ->default(env("APP_API_{$envPrefix}_RATELIMIT", config("http.rate_limit.{$configKey}"))), TextInput::make("APP_API_{$envPrefix}_RATELIMIT_PERIOD") ->label('Period Length') ->required() ->numeric() ->minValue(1) ->suffix('Minutes') ->default(env("APP_API_{$envPrefix}_RATELIMIT_PERIOD", config("http.rate_limit.{$configKey}_period"))), Text::make($scopeDescription), ]); }Then call it as:
$this->makeRateLimitFieldset('Client API Ratelimit', 'CLIENT', 'client', 'Ratelimit is per user, or IP if there is no user'), $this->makeRateLimitFieldset('Application API Ratelimit', 'APPLICATION', 'application', 'Ratelimit is per user, or IP if there is no user'), // ... etcThis is optional since the current implementation is functional and clear, but it would improve maintainability if rate limit categories are added or modified in the future.
🤖 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 `@app/Filament/Admin/Pages/Settings.php` around lines 803 - 964, The repeated Fieldset::make() blocks in Settings.php implement the same pattern for various rate limits; extract that pattern into a private helper (e.g. makeRateLimitFieldset) that accepts parameters like $label, $envPrefix, $configKey, and $scopeDescription and returns a Fieldset, then replace each repeated block (the Client/Application/Password Reset/Websocket/Backup Restore/Database Create/Subuser Create/File Pull/Default blocks) with calls to this helper inside the form schema; ensure the helper constructs the two TextInput keys APP_API_{ENV}_RATELIMIT and APP_API_{ENV}_RATELIMIT_PERIOD and uses env(..., config(...)) for defaults and preserves the Text scope description.
803-964: ⚡ Quick winPlan to migrate hardcoded strings to translation system.
All labels, helper text, and scope descriptions throughout the API rate limit fieldsets use hardcoded English strings instead of
trans()calls (e.g.,'Client API Ratelimit','Requests Per Period','Ratelimit is per user, or IP if there is no user'). This blocks internationalization.While the PR description acknowledges this is temporary and will be addressed once the design is finalized, ensure this migration happens before the feature is considered complete to maintain consistency with the rest of the settings page (which uses
trans('admin/setting...')throughout).🤖 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 `@app/Filament/Admin/Pages/Settings.php` around lines 803 - 964, The rate limit fieldsets use hardcoded English strings; replace each literal label and helper text with translation calls (e.g., trans(...)) so the UI is localizable: update Fieldset::make(...) labels like 'Client API Ratelimit', TextInput::make(...) ->label('Requests Per Period'), ->suffix('Minutes'/'Requests') and Text::make(...) helper lines inside the blocks for APP_API_CLIENT_RATELIMIT, APP_API_CLIENT_RATELIMIT_PERIOD, APP_API_APPLICATION_RATELIMIT, APP_API_APPLICATION_RATELIMIT_PERIOD, APP_API_PASSWORD_RESET_RATELIMIT, APP_API_PASSWORD_RESET_RATELIMIT_PERIOD, APP_API_WEBSOCKET_RATELIMIT, APP_API_WEBSOCKET_RATELIMIT_PERIOD, APP_API_BACKUP_RESTORE_RATELIMIT, APP_API_BACKUP_RESTORE_RATELIMIT_PERIOD, APP_API_DATABASE_CREATE_RATELIMIT, APP_API_DATABASE_CREATE_RATELIMIT_PERIOD, APP_API_SUBUSER_CREATE_RATELIMIT, APP_API_SUBUSER_CREATE_RATELIMIT_PERIOD, APP_API_FILE_PULL_RATELIMIT, APP_API_FILE_PULL_RATELIMIT_PERIOD, APP_API_DEFAULT_RATELIMIT, and APP_API_DEFAULT_RATELIMIT_PERIOD to use trans('admin/settings.<meaningful_key>') (create matching translation keys for section labels, field labels, suffixes and helper descriptions).
🤖 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.
Nitpick comments:
In `@app/Filament/Admin/Pages/Settings.php`:
- Around line 803-964: The repeated Fieldset::make() blocks in Settings.php
implement the same pattern for various rate limits; extract that pattern into a
private helper (e.g. makeRateLimitFieldset) that accepts parameters like $label,
$envPrefix, $configKey, and $scopeDescription and returns a Fieldset, then
replace each repeated block (the Client/Application/Password
Reset/Websocket/Backup Restore/Database Create/Subuser Create/File Pull/Default
blocks) with calls to this helper inside the form schema; ensure the helper
constructs the two TextInput keys APP_API_{ENV}_RATELIMIT and
APP_API_{ENV}_RATELIMIT_PERIOD and uses env(..., config(...)) for defaults and
preserves the Text scope description.
- Around line 803-964: The rate limit fieldsets use hardcoded English strings;
replace each literal label and helper text with translation calls (e.g.,
trans(...)) so the UI is localizable: update Fieldset::make(...) labels like
'Client API Ratelimit', TextInput::make(...) ->label('Requests Per Period'),
->suffix('Minutes'/'Requests') and Text::make(...) helper lines inside the
blocks for APP_API_CLIENT_RATELIMIT, APP_API_CLIENT_RATELIMIT_PERIOD,
APP_API_APPLICATION_RATELIMIT, APP_API_APPLICATION_RATELIMIT_PERIOD,
APP_API_PASSWORD_RESET_RATELIMIT, APP_API_PASSWORD_RESET_RATELIMIT_PERIOD,
APP_API_WEBSOCKET_RATELIMIT, APP_API_WEBSOCKET_RATELIMIT_PERIOD,
APP_API_BACKUP_RESTORE_RATELIMIT, APP_API_BACKUP_RESTORE_RATELIMIT_PERIOD,
APP_API_DATABASE_CREATE_RATELIMIT, APP_API_DATABASE_CREATE_RATELIMIT_PERIOD,
APP_API_SUBUSER_CREATE_RATELIMIT, APP_API_SUBUSER_CREATE_RATELIMIT_PERIOD,
APP_API_FILE_PULL_RATELIMIT, APP_API_FILE_PULL_RATELIMIT_PERIOD,
APP_API_DEFAULT_RATELIMIT, and APP_API_DEFAULT_RATELIMIT_PERIOD to use
trans('admin/settings.<meaningful_key>') (create matching translation keys for
section labels, field labels, suffixes and helper descriptions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d23e871f-1faa-4ab8-897b-18e71337047c
📒 Files selected for processing (3)
app/Filament/Admin/Pages/Settings.phpapp/Providers/RouteServiceProvider.phpconfig/http.php
💤 Files with no reviewable changes (2)
- app/Providers/RouteServiceProvider.php
- config/http.php
|
something important, i do believe this currently overrides the values (ie not using a translation key). i was hoping to get confirmation that the design of the ui and wording is good before changing it to use translation keys |
|
working on the above translation key stuff, might as well get it done now rather than later |
|
i have made a few python scripts in a jupyter notebook to help auto generate certain things, such as for the translation key file and the code for the ui, lots of boiler plate and all of that. |
There 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 `@lang/en/admin/setting.php`:
- Around line 142-146: Update the 'helper' text for the API rate limit entry so
it no longer mentions "per minute" and instead explains the new per-period
model; locate the 'helper' key in the same block as 'requests_per_period',
'requests', 'minutes', and 'period_length' and replace the copy with a concise
description like "Defines the rate limit as the number of requests allowed per
configured period (Requests Per Period and Period Length control this)." Ensure
the new string aligns with the existing language format and keys 'helper',
'requests_per_period', and 'period_length'.
🪄 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: 640cd62f-0ea4-4be7-b0d9-f827bebcf338
📒 Files selected for processing (2)
app/Filament/Admin/Pages/Settings.phplang/en/admin/setting.php
4 participants
i recently moved some ratelimits to env vars to allow better configurability. i decided to try my hand at adding an ui area to configure them. i attached an img below showing what it looks like
the main changes are from a flat requests per minute for rate-limits to a requests in a time period, i feel this better fits with the ability to have the period being more than a minute
i added a small description text on them to give more info about them, currently just what the ratelimit is per.
once the format/design, and the names/wording is confirmed i will work on moving the names and such to the translation thing. As i don't know languages other than english, i will not be able to get translations for other languages
due to how long/tall the list of configs are, i'm wondering if we might want to make it go across the entire panel rather than only being on one half, not sure if that be proper for the design though, there is also the idea of more collapsable sections, but again, not sure if it would fit in the design and be good ux.