Merged
Conversation
Contributor
Author
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/code/src/renderer/features/sessions/stores/sessionStore.test.ts
Line: 26-65
Comment:
**Missing edge case: current value is filtered out**
The test suite doesn't cover the case where `currentValue` is itself a restricted mode (e.g. `"bypassPermissions"`) while `allowBypassPermissions` is `false`. In that scenario `findIndex` returns `-1` and `filtered[0]` is returned — falling back to the first mode. This may or may not be the intended UX, but it's worth having an explicit test case to document the expected behavior, particularly if a user's permissions are revoked mid-session.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: add back shift-tab support" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+26
to
+65
| it.each([ | ||
| { | ||
| name: "claude: advances to next mode when bypass allowed", | ||
| values: CLAUDE_MODES, | ||
| currentValue: "plan", | ||
| allowBypassPermissions: true, | ||
| expected: "bypassPermissions", | ||
| }, | ||
| { | ||
| name: "codex: advances to next mode when bypass allowed", | ||
| values: CODEX_MODES, | ||
| currentValue: "auto", | ||
| allowBypassPermissions: true, | ||
| expected: "full-access", | ||
| }, | ||
| { | ||
| name: "claude: skips bypassPermissions when not allowed", | ||
| values: CLAUDE_MODES, | ||
| currentValue: "acceptEdits", | ||
| allowBypassPermissions: false, | ||
| expected: "plan", | ||
| }, | ||
| { | ||
| name: "claude: wraps past bypassPermissions back to default", | ||
| values: CLAUDE_MODES, | ||
| currentValue: "plan", | ||
| allowBypassPermissions: false, | ||
| expected: "default", | ||
| }, | ||
| { | ||
| name: "codex: skips full-access when not allowed", | ||
| values: CODEX_MODES, | ||
| currentValue: "auto", | ||
| allowBypassPermissions: false, | ||
| expected: "read-only", | ||
| }, | ||
| ])("$name", ({ values, currentValue, allowBypassPermissions, expected }) => { | ||
| const option = createModeOption(currentValue, values); | ||
|
|
||
| expect(cycleModeOption(option)).toBe("full-access"); | ||
| expect(cycleModeOption(option, { allowBypassPermissions })).toBe(expected); |
There was a problem hiding this comment.
The test suite doesn't cover the case where currentValue is itself a restricted mode (e.g. "bypassPermissions") while allowBypassPermissions is false. In that scenario findIndex returns -1 and filtered[0] is returned — falling back to the first mode. This may or may not be the intended UX, but it's worth having an explicit test case to document the expected behavior, particularly if a user's permissions are revoked mid-session.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sessions/stores/sessionStore.test.ts
Line: 26-65
Comment:
**Missing edge case: current value is filtered out**
The test suite doesn't cover the case where `currentValue` is itself a restricted mode (e.g. `"bypassPermissions"`) while `allowBypassPermissions` is `false`. In that scenario `findIndex` returns `-1` and `filtered[0]` is returned — falling back to the first mode. This may or may not be the intended UX, but it's worth having an explicit test case to document the expected behavior, particularly if a user's permissions are revoked mid-session.
How can I resolve this? If you propose a fix, please make it concise.
This was referenced Apr 22, 2026
jonathanlab
force-pushed
the
04-22-fix_add_back_shift-tab_support
branch
from
84582fa to
fdbba1e
Compare
jonathanlab
force-pushed
the
04-22-fix_handle_git-lfs_with_git_lfs_skip_smudge
branch
from
63b3f48 to
e5259af
Compare
This was referenced Apr 22, 2026
adboio
approved these changes
Apr 22, 2026
Member
Merge activity
|
charlesvien
changed the base branch from
04-22-fix_handle_git-lfs_with_git_lfs_skip_smudge
to
graphite-base/1805
charlesvien
force-pushed
the
04-22-fix_add_back_shift-tab_support
branch
from
fdbba1e to
d3cf85e
Compare
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
3 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.
Closes #1773