Preserve state keys#1897
Open
McHersheys wants to merge 1 commit into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
License agreement recorded@McHersheys, your agreement has been recorded and you have been added to contributors.md. All future PRs from your account will pass this check automatically. |
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesState item normalization and tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
![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: 2
🧹 Nitpick comments (1)
console/packages/console-frontend/src/api/state/state.test.ts (1)
32-39: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression case for
value.keybeating a positional map key.This test only proves plain map keys are used. It would not catch the
/statesregression where an object entry likeitem-0masks an embedded semantickey.Proposed test addition
it('uses map keys for object-shaped item responses', async () => { mockStateItems({ items: { 'files/conscience/covenant.yaml': { raw: true } } }) const { items } = await fetchStateItems('chambers') expect(items[0].key).toBe('files/conscience/covenant.yaml') expect(items[0].value).toEqual({ raw: true }) }) + + it('uses explicit item keys before object entry keys', async () => { + mockStateItems({ items: { 'item-0': { key: 'worker', value: { status: 'ready' } } } }) + + const { items } = await fetchStateItems('chambers') + + expect(items[0].key).toBe('worker') + })🤖 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 `@console/packages/console-frontend/src/api/state/state.test.ts` around lines 32 - 39, Add a regression test case to the test file that verifies when a value object contains its own `key` property, the actual map key is still correctly used and not overridden by the embedded key. Extend or create a new test in the state.test.ts file that mocks a response where an item value contains a `key` property (for example, `{ raw: true, key: 'some-other-key' }`), and assert that `items[0].key` resolves to the map key (the object property name) and not the embedded `value.key` property. This regression test should prevent regressions like the `/states` issue where an embedded `key` property could incorrectly mask the positional map key.
🤖 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 `@console/packages/console-frontend/src/api/state/state.ts`:
- Around line 33-35: The stateItemValue function only unwraps item.value when
item.key or item.state_key are non-empty strings, but the code elsewhere accepts
item.id as a valid key source. Update the condition in the stateItemValue
function to also check nonEmptyString(item.id) alongside the existing checks for
item.key and item.state_key, so that the value is properly unwrapped regardless
of which key source is used.
- Around line 76-80: In the state.ts file within the map function that processes
rawItems, the priority order for selecting a key is incorrect. The code
currently uses "key || embeddedKey || fallbackKey(index)" which prioritizes the
Object.entries key before the embedded value.key property. Change the return
statement in makeStateItem to prioritize embeddedKey (which already checks
value.key first via nonEmptyString calls) over the map entry key, so the order
becomes "embeddedKey || key || fallbackKey(index)" instead. This ensures that an
explicit value.key property in the record takes precedence over the map entry
key before falling back to other alternatives.
---
Nitpick comments:
In `@console/packages/console-frontend/src/api/state/state.test.ts`:
- Around line 32-39: Add a regression test case to the test file that verifies
when a value object contains its own `key` property, the actual map key is still
correctly used and not overridden by the embedded key. Extend or create a new
test in the state.test.ts file that mocks a response where an item value
contains a `key` property (for example, `{ raw: true, key: 'some-other-key' }`),
and assert that `items[0].key` resolves to the map key (the object property
name) and not the embedded `value.key` property. This regression test should
prevent regressions like the `/states` issue where an embedded `key` property
could incorrectly mask the positional map key.
🪄 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: d38bc76f-66d8-4bf9-bd67-c3fdcb5b499e
📒 Files selected for processing (2)
console/packages/console-frontend/src/api/state/state.test.tsconsole/packages/console-frontend/src/api/state/state.ts
Comment on lines
+33
to
+35
| function stateItemValue(item: Record<string, unknown>): unknown { | ||
| if ('value' in item && (nonEmptyString(item.key) || nonEmptyString(item.state_key))) { | ||
| return item.value |
Contributor
There was a problem hiding this comment.
Unwrap value consistently when id is used as the key.
Line 63 accepts item.id as a key source, but Line 34 only unwraps item.value for key/state_key. { id: 'worker', value: {...} } would render the wrapper object instead of the actual state value.
Proposed fix
function stateItemValue(item: Record<string, unknown>): unknown {
- if ('value' in item && (nonEmptyString(item.key) || nonEmptyString(item.state_key))) {
+ if (
+ 'value' in item &&
+ (nonEmptyString(item.key) || nonEmptyString(item.state_key) || nonEmptyString(item.id))
+ ) {
return item.value
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function stateItemValue(item: Record<string, unknown>): unknown { | |
| if ('value' in item && (nonEmptyString(item.key) || nonEmptyString(item.state_key))) { | |
| return item.value | |
| function stateItemValue(item: Record<string, unknown>): unknown { | |
| if ( | |
| 'value' in item && | |
| (nonEmptyString(item.key) || nonEmptyString(item.state_key) || nonEmptyString(item.id)) | |
| ) { | |
| return item.value | |
| } | |
| } |
🤖 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 `@console/packages/console-frontend/src/api/state/state.ts` around lines 33 -
35, The stateItemValue function only unwraps item.value when item.key or
item.state_key are non-empty strings, but the code elsewhere accepts item.id as
a valid key source. Update the condition in the stateItemValue function to also
check nonEmptyString(item.id) alongside the existing checks for item.key and
item.state_key, so that the value is properly unwrapped regardless of which key
source is used.
Comment on lines
+76
to
+80
| return Object.entries(rawItems).map(([key, value], index) => { | ||
| const embeddedKey = isRecord(value) | ||
| ? (nonEmptyString(value.key) ?? nonEmptyString(value.state_key) ?? nonEmptyString(value.id)) | ||
| : null | ||
| return makeStateItem(groupId, key || embeddedKey || fallbackKey(index), value) |
Contributor
There was a problem hiding this comment.
Honor explicit value.key before the map entry key.
Line 80 currently chooses the Object.entries(...) key first, so { items: { 'item-0': { key: 'worker' } } } would still display item-0. The requested priority puts item.key before tuple/map keys.
Proposed fix
if (isRecord(rawItems)) {
return Object.entries(rawItems).map(([key, value], index) => {
- const embeddedKey = isRecord(value)
- ? (nonEmptyString(value.key) ?? nonEmptyString(value.state_key) ?? nonEmptyString(value.id))
+ const valueRecord = isRecord(value) ? value : null
+ const explicitKey = valueRecord ? nonEmptyString(valueRecord.key) : null
+ const embeddedKey = valueRecord
+ ? (nonEmptyString(valueRecord.state_key) ?? nonEmptyString(valueRecord.id))
: null
- return makeStateItem(groupId, key || embeddedKey || fallbackKey(index), value)
+ return makeStateItem(
+ groupId,
+ explicitKey ?? nonEmptyString(key) ?? embeddedKey ?? fallbackKey(index),
+ value,
+ )
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return Object.entries(rawItems).map(([key, value], index) => { | |
| const embeddedKey = isRecord(value) | |
| ? (nonEmptyString(value.key) ?? nonEmptyString(value.state_key) ?? nonEmptyString(value.id)) | |
| : null | |
| return makeStateItem(groupId, key || embeddedKey || fallbackKey(index), value) | |
| return Object.entries(rawItems).map(([key, value], index) => { | |
| const valueRecord = isRecord(value) ? value : null | |
| const explicitKey = valueRecord ? nonEmptyString(valueRecord.key) : null | |
| const embeddedKey = valueRecord | |
| ? (nonEmptyString(valueRecord.state_key) ?? nonEmptyString(valueRecord.id)) | |
| : null | |
| return makeStateItem( | |
| groupId, | |
| explicitKey ?? nonEmptyString(key) ?? embeddedKey ?? fallbackKey(index), | |
| value, | |
| ) |
🤖 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 `@console/packages/console-frontend/src/api/state/state.ts` around lines 76 -
80, In the state.ts file within the map function that processes rawItems, the
priority order for selecting a key is incorrect. The code currently uses "key ||
embeddedKey || fallbackKey(index)" which prioritizes the Object.entries key
before the embedded value.key property. Change the return statement in
makeStateItem to prioritize embeddedKey (which already checks value.key first
via nonEmptyString calls) over the map entry key, so the order becomes
"embeddedKey || key || fallbackKey(index)" instead. This ensures that an
explicit value.key property in the record takes precedence over the map entry
key before falling back to other alternatives.
McHersheys
force-pushed
the
fix/state-key-labels-1896
branch
from
95908ac to
fc62c41
Compare
Contributor
|
Thanks for the PR! We'll take a look and get back to you |
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.
Fixes #1896.
Tests:
corepack pnpm --filter console-frontend test -- src/api/state/state.test.tscorepack pnpm --filter console-frontend lintcorepack pnpm --filter console-frontend buildI license my contributions to this repository under Apache 2.0, and I have all necessary rights over the code I am contributing.
Summary by CodeRabbit
Improvements
Tests