Skip to content

Index: Remove hasActiveFilters check - #35151

Merged
Sidnioulz merged 2 commits into
nextfrom
bill/burn-has-active-filters
Jun 26, 2026
Merged

Sidnioulz merged 2 commits into
nextfrom
bill/burn-has-active-filters

Conversation

@mrginglymus

@mrginglymus mrginglymus commented Jun 12, 2026 •

Copy link
Copy Markdown
Contributor

What I did

There are multiple sources that would cause the index to be filtered, and working out which are active is effectively impossible.

Instead, just check for the presence of a filtered index and use that if available.

Addresses a bug reported on Discord, see https://discord.com/channels/486522875931656193/1507589559439986720 for the conversation and analysis.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  • Apply a custom filter to code/.storybook/manager.ts targeting the benchmark story
  • Verify the initially loaded story is not benchmark

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Declare whether manual QA will be needed for this PR during the next release, through qa:needed or qa:skip

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>

Summary by CodeRabbit

  • Refactor
    • Improved story initialization and navigation so the app prefers the first available filtered story when a filtered list is present, falling back to the unfiltered list otherwise.
    • Ensures canvas/story routes respect the filtered index selection logic consistently.
  • Tests
    • Added coverage to verify “select first story” navigates to the first filtered story even when status filters aren’t currently active.

@mrginglymus

Copy link
Copy Markdown
Contributor Author

Oh shush, danger - it's a draft PR :P

@Sidnioulz Sidnioulz changed the title Remove hasActiveFilters check Index: Remove hasActiveFilters check Jun 12, 2026
@Sidnioulz Sidnioulz added bug story index ci:normal Run our default set of CI jobs (choose this for most PRs). qa:needed Pull Requests that will need manual QA prior to release. labels Jun 12, 2026
@Sidnioulz

Copy link
Copy Markdown
Contributor

Oh shush, danger - it's a draft PR :P

Are you able to set labels? You should. Danger is out looking for fresh blood, you're our first PR of the day! It's hungry!

@storybook-app-bot

storybook-app-bot Bot commented Jun 12, 2026 •

Copy link
Copy Markdown
Contributor

Package Benchmarks

Commit: 1e729c8, ran on 26 June 2026 at 09:09:35 UTC

No significant changes detected, all good. 👏

@mrginglymus

Copy link
Copy Markdown
Contributor Author

Well that seemed to break...nothing? If endorsed I'll write some actual tests

@Sidnioulz Sidnioulz self-assigned this Jun 15, 2026
@Sidnioulz

Copy link
Copy Markdown
Contributor

LGTM @mrginglymus! Do you wanna follow up here on the other function or separately? I expect it'll be harder 😄

@mrginglymus

Copy link
Copy Markdown
Contributor Author

I think I'll chase that other one elsewhere, this fixes an immediate issue I have withour storybook. I'll try and add some tests tomorrow if I can find where...

@Sidnioulz

Copy link
Copy Markdown
Contributor

I think I'll chase that other one elsewhere, this fixes an immediate issue I have withour storybook. I'll try and add some tests tomorrow if I can find where...

You could start in the Explorer stories with a mocked filteredIndex, I think that's often how it's done. FilterPanel stories should also still have examples of data mocking to test filter UI. This would prove the Explorer uses the right data structure.

I don't think we have a good place to test initial story loading behaviour without writing a whole sandbox. You could add an IntentionallyFilteredOut story to the sandbox templates and then add a line in the manager that applies a custom filter targeting this story. Then an E2E test would only check if the story exists in the sidebar (you can try to locate its generated id or test ID, there are examples of that).

@mrginglymus
mrginglymus force-pushed the bill/burn-has-active-filters branch 2 times, most recently from 0457e9b to ed75069 Compare June 21, 2026 07:29
@mrginglymus
mrginglymus marked this pull request as ready for review June 21, 2026 07:29
@mrginglymus

Copy link
Copy Markdown
Contributor Author

@Sidnioulz found some tests to add a test to.

@coderabbitai

coderabbitai Bot commented Jun 21, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9e8e757a-db7c-465c-9858-ed4850f8011b

📥 Commits

Reviewing files that changed from the base of the PR and between 0fd2629 and 1e729c8.

📒 Files selected for processing (3)
  • code/core/src/manager-api/modules/stories.ts
  • code/core/src/manager-api/tests/stories.test.ts
  • code/core/src/shared/utils/story-index-filters.ts
💤 Files with no reviewable changes (1)
  • code/core/src/shared/utils/story-index-filters.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • code/core/src/manager-api/tests/stories.test.ts
  • code/core/src/manager-api/modules/stories.ts

📝 Walkthrough

Walkthrough

The hasActiveFilters helper is removed. Story selection logic now checks state.filteredIndex directly in stories.ts, and a test covers the filtered selection path when status filters are empty.

Changes

Filter presence check refactor

Layer / File(s) Summary
Remove hasActiveFilters utility and import
code/core/src/shared/utils/story-index-filters.ts, code/core/src/manager-api/modules/stories.ts
Deletes the exported hasActiveFilters function and removes its import from stories.ts.
Use filteredIndex in story selection
code/core/src/manager-api/modules/stories.ts, code/core/src/manager-api/tests/stories.test.ts
selectFirstStory now branches on state.filteredIndex, and the STORY_SPECIFIED handler checks filteredIndex directly with !stateHasSelection. A test asserts navigation to /story/a--2 when status filter arrays are empty.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
code/core/src/manager-api/tests/stories.test.ts (1)

2059-2080: ⚡ Quick win

Add a companion test for the STORY_SPECIFIED filtered-index path.

This new case covers selectFirstStory, but the other changed branch in STORY_SPECIFIED (filteredIndex && !stateHasSelection) is still unasserted. A focused channel-event test there would lock in the bug fix behavior on initial canvas routing.

🤖 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 `@code/core/src/manager-api/tests/stories.test.ts` around lines 2059 - 2080,
Add a new test case in the stories.test.ts file that specifically tests the
STORY_SPECIFIED action path when filteredIndex is available and
stateHasSelection is false. This test should verify the behavior for initial
canvas routing by setting up the moduleArgs with a STORY_SPECIFIED action, an
active filteredIndex, no current story selection, and then asserting that the
navigate function is called with the first story from the filteredIndex rather
than from the full index. This will ensure the bug fix behavior for the
filteredIndex && !stateHasSelection branch in the STORY_SPECIFIED handler is
properly locked in.
🤖 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 `@code/core/src/manager-api/tests/stories.test.ts`:
- Around line 2059-2080: Add a new test case in the stories.test.ts file that
specifically tests the STORY_SPECIFIED action path when filteredIndex is
available and stateHasSelection is false. This test should verify the behavior
for initial canvas routing by setting up the moduleArgs with a STORY_SPECIFIED
action, an active filteredIndex, no current story selection, and then asserting
that the navigate function is called with the first story from the filteredIndex
rather than from the full index. This will ensure the bug fix behavior for the
filteredIndex && !stateHasSelection branch in the STORY_SPECIFIED handler is
properly locked in.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1b61a8fb-ac13-4b83-8115-771094e11ab3

📥 Commits

Reviewing files that changed from the base of the PR and between 7e7251e and ed75069.

📒 Files selected for processing (3)
  • code/core/src/manager-api/modules/stories.ts
  • code/core/src/manager-api/tests/stories.test.ts
  • code/core/src/shared/utils/story-index-filters.ts
💤 Files with no reviewable changes (1)
  • code/core/src/shared/utils/story-index-filters.ts

mrginglymus and others added 2 commits June 26, 2026 10:52
There are multiple sources that would cause the index to be filtered,
and working out which are active is effectively impossible.

Instead, just check for the presence of a filtered index and use that if
available.
@Sidnioulz
Sidnioulz force-pushed the bill/burn-has-active-filters branch from 0fd2629 to 1e729c8 Compare June 26, 2026 08:52
@Sidnioulz
Sidnioulz merged commit de136b8 into next Jun 26, 2026
154 checks passed
@Sidnioulz
Sidnioulz deleted the bill/burn-has-active-filters branch June 26, 2026 09:32
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Core Team Projects Jun 26, 2026
@github-actions github-actions Bot mentioned this pull request Jun 26, 2026
2 tasks done
This was referenced Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal Run our default set of CI jobs (choose this for most PRs). qa:failing Pull requests with a known QA issue story index

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants