Fix App dataset color scheme assignment #5478
Conversation
WalkthroughThe pull request refactors how session updates occur by extracting the inline logic into a new function called assignSession. This function centralizes the updating of the session object by setting default values and using the ensureColorScheme utility. In addition, a new Vitest test suite confirms that invoking assignSession correctly updates the session’s color scheme—specifically the colorPool property—when transitioning datasets, while preserving the existing unsubscribe behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Test Suite
participant Client as assignSession Caller
participant AS as assignSession
participant ECS as ensureColorScheme
participant Sub as Subscription
Tester->>Client: Initialize session & settings
Client->>AS: Call assignSession(session, settings)
AS->>ECS: Update colorScheme settings
ECS-->>AS: Return updated colorScheme
AS->>Sub: Unsubscribe from session updates
AS-->>Client: Return updated session
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/packages/app/src/useSetters/onSetDataset.test.ts (1)
5-12: Enhance test coverage for assignSession function.While the basic color scheme test is good, consider adding tests for:
- Other session properties (selectedLabels, selectedSamples, etc.)
- Edge cases (null/undefined settings)
- Config parameter effects on color scheme
Here's a suggested test expansion:
describe("test session assignment for dataset transition", () => { it("assigns color scheme settings", () => { const session = { ...SESSION_DEFAULT }; assignSession(session, { colorScheme: { colorPool: ["#ffffff"] } }); expect(session.colorScheme.colorPool).toEqual(["#ffffff"]); }); it("resets session properties", () => { const session = { ...SESSION_DEFAULT, selectedLabels: ["label1"], selectedSamples: new Set(["sample1"]), fieldVisibilityStage: "some_stage" }; assignSession(session, {}); expect(session.selectedLabels).toEqual([]); expect(session.selectedSamples.size).toBe(0); expect(session.fieldVisibilityStage).toBeUndefined(); }); it("handles undefined settings gracefully", () => { const session = { ...SESSION_DEFAULT }; assignSession(session, {}); expect(session.sessionGroupSlice).toBeUndefined(); }); });app/packages/app/src/useSetters/onSetDataset.ts (1)
59-70: Simplify type definition for better readability.Consider extracting the complex nested type into a separate type alias for better maintainability.
type DatasetAppConfig = NonNullable<NonNullable<ResponseFrom<DatasetPageQuery>["dataset"]>["appConfig"]>; type SessionSettings = { colorScheme?: Partial<DatasetAppConfig["colorScheme"]>; config?: ResponseFrom<DatasetPageQuery>["config"]; groupSlice?: null | string; }; export const assignSession = ( session: Session, settings: SessionSettings ) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/app/src/useSetters/onSetDataset.test.ts(1 hunks)app/packages/app/src/useSetters/onSetDataset.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/app/src/useSetters/onSetDataset.tsapp/packages/app/src/useSetters/onSetDataset.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
🔇 Additional comments (2)
app/packages/app/src/useSetters/onSetDataset.ts (2)
31-38: LGTM! Clean implementation of session assignment.The refactoring improves code organization by centralizing session update logic while maintaining the existing unsubscribe behavior.
71-80: LGTM! Clear session reset and update logic.The implementation properly resets session state and applies new settings in a predictable order.
What changes are proposed in this pull request?
Minimal change to fix #5410 with a test
To reproduce
Recording shows correct behavior
Screen.Recording.2025-02-07.at.4.35.48.PM.mov
How is this patch tested? If it is not, please explain why.
Test case for
assignSessionRelease Notes
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit