Skip to content

[dashboards] Fix source filter firing navigation on every keystroke#131

Merged
matthallett1 merged 2 commits into
NateBJones-Projects:mainfrom
swhitt:contrib/swhitt/fix-source-filter-keystroke
Mar 30, 2026
Merged

[dashboards] Fix source filter firing navigation on every keystroke#131
matthallett1 merged 2 commits into
NateBJones-Projects:mainfrom
swhitt:contrib/swhitt/fix-source-filter-keystroke

Conversation

@swhitt

@swhitt swhitt commented Mar 28, 2026

Copy link
Copy Markdown
Contributor

Contribution Type

  • Dashboard (/dashboards)

What does this do?

The Source text input in ThoughtsFilter calls router.push() on every onChange event, triggering a full server-side navigation for each character typed. Typing "chatgpt_import" fires 16 navigations in rapid succession.

Fix: use local state for the input value, apply the filter on blur or Enter. The select-based filters (Type, Importance) are unaffected since they commit on selection, not per-keystroke.

Requirements

Nothing new.

Checklist

  • I've read CONTRIBUTING.md
  • My contribution has a README.md with prerequisites, step-by-step instructions, and expected outcome (n/a - see PR body)
  • My metadata.json has all required fields (n/a)
  • I tested this on my own Open Brain instance
  • No credentials, API keys, or secrets are included

README and metadata.json not applicable for a single-file bug fix.

@shansen3

Copy link
Copy Markdown

Code review

Found 1 issue:

  1. State desync when URL changes externally. sourceInput is initialized once via useState(currentSource) but never syncs back when currentSource changes (e.g., browser back/forward, another filter clearing the URL param). The text input will show a stale value that doesn't match the active filter. Add a useEffect to sync: useEffect(() => { setSourceInput(currentSource ?? ""); }, [currentSource]);

const router = useRouter();
const [sourceInput, setSourceInput] = useState(currentSource);

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Fixes stale input state on browser back/forward or external URL
changes, as caught by @shansen3.
@swhitt

swhitt commented Mar 29, 2026

Copy link
Copy Markdown
Contributor Author

Good catch, thanks. Fixed in 253560e. Added a useEffect to sync sourceInput when currentSource changes externally (back/forward, URL edits).

@matthallett1

Copy link
Copy Markdown
Collaborator

@claude review this PR

1 similar comment
@matthallett1

Copy link
Copy Markdown
Collaborator

@claude review this PR

@github-actions

github-actions Bot commented Mar 30, 2026

Copy link
Copy Markdown

Claude finished @matthallett1's task in 46s —— View job


Code Review

  • Read changed file
  • Review implementation
  • Post findings

The fix is correct and well-scoped. Here's my assessment:

What changed (ThoughtsFilter.tsx):

  • sourceInput local state buffers keystrokes instead of hitting the router on every onChange
  • Filter applies on blur (with a guard: only if value differs from currentSource) or on Enter
  • A useEffect syncs sourceInput back to currentSource when the URL changes externally (back/forward nav, other filters clearing the param) — this addresses the issue that was caught and fixed in 253560e

The implementation is sound. A few minor observations:

  1. onKeyDown Enter doesn't guard against no-op — unlike onBlur, pressing Enter when sourceInput === currentSource still calls applyFilters. This is harmless (it pushes the same URL) but slightly inconsistent with the blur guard.

  2. useEffect is one-liner — inline form on line 22 is compact but slightly harder to scan. Not a blocker.

  3. currentSource prop typed as string but used with ?? "" in applyFilters — prop type should be string | undefined to match actual usage, or the nullish coalescing should be removed. Minor inconsistency.

Checklist:

  • Alignment: yes — dashboard bug fix, directly improves existing UX
  • Safety: no credentials, no SQL, no schema changes
  • Scope: appropriately minimal — single-file fix
  • Prior review addressed: yes — the useEffect sync is in place

Verdict: LGTM. The two minor items above are worth noting but neither blocks merging. The core fix is correct, the state desync from the previous review has been addressed, and the UX is meaningfully improved.


@matthallett1 matthallett1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Clean bugfix — local state + blur/Enter commit is the standard React pattern. State desync addressed in 253560e. Reviewed with gstack /review and automated review, no issues.

@matthallett1 matthallett1 merged commit b1d2c88 into NateBJones-Projects:main Mar 30, 2026
gleesonb pushed a commit to gleesonb/OB1 that referenced this pull request May 12, 2026
…ateBJones-Projects#131)

* [dashboards] Fix source filter firing navigation on every keystroke

* Sync sourceInput when currentSource changes externally

Fixes stale input state on browser back/forward or external URL
changes, as caught by @shansen3.
thedataengineer pushed a commit to yadavilli-solutions/OB1 that referenced this pull request May 29, 2026
…ateBJones-Projects#131)

* [dashboards] Fix source filter firing navigation on every keystroke

* Sync sourceInput when currentSource changes externally

Fixes stale input state on browser back/forward or external URL
changes, as caught by @shansen3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants