fix: e2e ui tests for model limts - #4519
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe e2e model-limits page adds a ChangesModel Limits E2E Page Helper Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Jun 18, 2026
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sammaji
marked this pull request as ready for review
June 18, 2026 08:26
Contributor
Confidence Score: 5/5Scoped to a single E2E page object with no production code changes; safe to merge. The changes are confined to one test helper file. The logic is straightforward: combobox interaction replaces the old multiselect input flow, and the new setBudget method correctly branches between adding a fresh budget line and updating an existing one. The only finding is a dead escapeRegex function with no call sites, which has no impact on test correctness or runtime behavior. No files require special attention beyond confirming the resetPeriodLabels keys match whatever duration values the test data passes in. Important Files Changed
Reviews (4): Last reviewed commit: "fix: e2e ui tests for model limts" | Re-trigger Greptile |
![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)
tests/e2e/features/model-limits/pages/model-limits.page.ts (1)
18-21: ⚡ Quick winAlign
ModelLimitConfig/docs with the new “All Models only” behavior.Line 20 still requires
modelName, and Lines 110-113 still describe selectingconfig.modelName, but Lines 126-132 now always select “All Models” and return'*'. This contract drift will confuse callers and future test updates.Suggested cleanup
export interface ModelLimitConfig { provider: string - modelName: string + // Current UI flow only supports wildcard model limit creation. + modelName?: string budget?: { maxLimit: number; resetDuration?: string } rateLimit?: { tokenMaxLimit?: number requestMaxLimit?: number } } /** - * Create a model limit via the sheet: selects provider, selects the requested - * model (config.modelName) in the search dropdown, fills budget and rate - * limit, then saves. Returns the selected model name for use in exists/edit/delete. + * Create a model limit via the sheet: selects provider, selects "All Models", + * fills budget and rate limit, then saves. Returns '*' for exists/edit/delete lookups. */Also applies to: 109-113, 126-132
🤖 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 `@tests/e2e/features/model-limits/pages/model-limits.page.ts` around lines 18 - 21, The ModelLimitConfig interface requires modelName as a mandatory field, but the implementation in the function that uses this config (around lines 126-132) always selects "All Models" and returns '*', ignoring the modelName value. Additionally, the comments at lines 110-113 describe selecting config.modelName which no longer matches the actual behavior. Make the modelName property optional in the ModelLimitConfig interface by adding a question mark, and update all documentation and comments that reference selecting config.modelName to clarify that "All Models" is always selected regardless of the provided modelName value.
🤖 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 `@tests/e2e/features/model-limits/pages/model-limits.page.ts`:
- Around line 101-106: The code in setBudget method currently uses a fallback to
the raw config.resetDuration value when it's not found in resetPeriodLabels,
which causes silent failures and timeouts during option lookup. Add an explicit
guard that throws an error with a clear message immediately after attempting to
retrieve resetPeriodLabel from resetPeriodLabels, ensuring that unsupported
resetDuration values fail fast with actionable feedback instead of timing out
during the getByRole option click.
---
Nitpick comments:
In `@tests/e2e/features/model-limits/pages/model-limits.page.ts`:
- Around line 18-21: The ModelLimitConfig interface requires modelName as a
mandatory field, but the implementation in the function that uses this config
(around lines 126-132) always selects "All Models" and returns '*', ignoring the
modelName value. Additionally, the comments at lines 110-113 describe selecting
config.modelName which no longer matches the actual behavior. Make the modelName
property optional in the ModelLimitConfig interface by adding a question mark,
and update all documentation and comments that reference selecting
config.modelName to clarify that "All Models" is always selected regardless of
the provided modelName value.
🪄 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 Plus
Run ID: cc175b80-33d1-4964-8b99-0c8906a85d53
📒 Files selected for processing (1)
tests/e2e/features/model-limits/pages/model-limits.page.ts
sammaji
force-pushed
the
06-18-fix_e2e_ui_tests_for_model_limts
branch
from
June 18, 2026 08:54
c868296 to
d8f4dbf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/features/model-limits/pages/model-limits.page.ts (2)
146-149: 💤 Low valuePrefer
data-testidselectors over hardcoded IDs.Lines 146 and 149 use
#modelTokenMaxLimitand#modelRequestMaxLimitID selectors. Usingdata-testidattributes (like the rest of the page object) would improve resilience to UI refactoring.🤖 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 `@tests/e2e/features/model-limits/pages/model-limits.page.ts` around lines 146 - 149, Replace the hardcoded ID selectors in the fill operations for model limits with data-testid attributes to maintain consistency with the rest of the page object and improve resilience to UI changes. Specifically, change the locator selectors from using `#modelTokenMaxLimit` to use a data-testid attribute format and from `#modelRequestMaxLimit` to use a data-testid attribute format, following the same pattern used elsewhere in the model-limits.page.ts file.
139-141: ⚡ Quick winConsider adding verification after model selection.
After clicking the option and pressing
Tab, there's no assertion that the combobox accepted the selection. Adding a brief check (e.g., verify the combobox displays the expected value) would catch selection failures earlier and reduce flakiness.💡 Example verification
await targetOption.click() await this.page.keyboard.press('Tab') + await expect(modelInput).toHaveValue(searchTerm, { timeout: 3000 }) const selectedModelName = config.modelName🤖 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 `@tests/e2e/features/model-limits/pages/model-limits.page.ts` around lines 139 - 141, The code clicks on targetOption and presses Tab to select a model but doesn't verify the selection succeeded. After the Tab press, add an assertion to verify that the combobox element displays the expected selectedModelName value. This will catch selection failures early and prevent flaky tests. You can use a waitFor or expect statement to check that the combobox contains or displays the selected model name.
🤖 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 `@tests/e2e/features/model-limits/pages/model-limits.page.ts`:
- Around line 130-142: Add special case handling for when config.modelName is
the literal '*' character representing All Models. Before the
modelInput.pressSequentially call on line 134, check if config.modelName equals
'*' and if so, set a separate displayName variable to 'All Models' to use in the
regex search on lines 135-137, while typing a different input (such as 'All')
via pressSequentially that will properly display the All Models option in the
dropdown. This ensures the regex pattern and UI interaction match correctly when
the special All Models identifier is used.
---
Nitpick comments:
In `@tests/e2e/features/model-limits/pages/model-limits.page.ts`:
- Around line 146-149: Replace the hardcoded ID selectors in the fill operations
for model limits with data-testid attributes to maintain consistency with the
rest of the page object and improve resilience to UI changes. Specifically,
change the locator selectors from using `#modelTokenMaxLimit` to use a
data-testid attribute format and from `#modelRequestMaxLimit` to use a
data-testid attribute format, following the same pattern used elsewhere in the
model-limits.page.ts file.
- Around line 139-141: The code clicks on targetOption and presses Tab to select
a model but doesn't verify the selection succeeded. After the Tab press, add an
assertion to verify that the combobox element displays the expected
selectedModelName value. This will catch selection failures early and prevent
flaky tests. You can use a waitFor or expect statement to check that the
combobox contains or displays the selected model name.
🪄 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 Plus
Run ID: 346e1164-51a8-4c7e-82b1-87a3427bf47a
📒 Files selected for processing (1)
tests/e2e/features/model-limits/pages/model-limits.page.ts
sammaji
force-pushed
the
06-18-fix_e2e_ui_tests_for_model_limts
branch
from
June 19, 2026 09:36
d8f4dbf to
eee4550
Compare
sammaji
force-pushed
the
06-18-fix_e2e_ui_tests_for_providers_management
branch
from
June 19, 2026 09:36
64ffcd7 to
79c6471
Compare
18 tasks
Contributor
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 `@tests/e2e/features/model-limits/pages/model-limits.page.ts`:
- Around line 138-141: The RegExp pattern for matching the model name in the
getByRole call is unanchored, which allows partial matches like "gpt-4" matching
"gpt-4o", causing the wrong option to be selected. Anchor the regex pattern with
`^` at the start and `$` at the end in the expression `new
RegExp(escapeRegex(config.modelName))` to ensure it matches the exact model name
only, preventing incorrect option selection when calling `.first()`.
🪄 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 Plus
Run ID: 5f6c158d-a5a8-46ea-8498-3d8dfa18418f
📒 Files selected for processing (1)
tests/e2e/features/model-limits/pages/model-limits.page.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
sammaji
force-pushed
the
06-18-fix_e2e_ui_tests_for_model_limts
branch
from
June 19, 2026 10:48
eee4550 to
2ebd0ab
Compare
Contributor
Merge activity
|
akshaydeo
changed the base branch from
06-18-fix_e2e_ui_tests_for_providers_management
to
graphite-base/4519
June 19, 2026 18:10
akshaydeo
dismissed
coderabbitai[bot]’s stale review
June 19, 2026 18:10
The base branch was changed.
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
2 participants
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.
Summary
Updates the model limits E2E page object to align with the current UI, where the model selector now defaults to "All Models" via a combobox rather than a searchable multiselect. Also introduces a reusable
setBudgethelper that handles adding a budget line, filling the amount, and optionally selecting a reset period.Changes
'*'.setBudgetmethod used by bothcreateModelLimitandupdateModelLimit, replacing the previous inline#modelBudgetMaxLimitlocator usage.resetPeriodLabelsmap to translate duration shorthand keys (e.g.'1h','1d') into their human-readable dropdown labels (e.g.'Hourly','Daily') for selecting reset periods in the budget line combobox.Type of change
Affected areas
How to test
Run the model limits E2E tests and verify that limit creation and update flows complete without errors, including budget lines with reset periods.
Breaking changes
Related issues
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines