Skip to content

fix: page URL param now loads correct page and auto-opens request detail panel#5493

Open
replicas-connector[bot] wants to merge 1 commit intomainfrom
ENG-3785
Open

fix: page URL param now loads correct page and auto-opens request detail panel#5493
replicas-connector[bot] wants to merge 1 commit intomainfrom
ENG-3785

Conversation

@replicas-connector
Copy link
Contributor

Summary

This PR fixes two issues with the requests page URL parameters (ENG-3785):

  1. Page parameter now correctly loads the specified page - Previously, navigating to a URL like /requests?page=5 would always reset to page 1. This was because the TableFooter component's local page state wasn't syncing with the currentPage prop when it changed externally.

  2. Request detail panel auto-opens when requestId is in URL - When navigating to a URL with requestId=, the right panel now properly opens. This was fixed by ensuring the drawer opens with a minimum size of 33% even if the stored drawerSize was 0.

Changes

web/components/templates/requests/tableFooter.tsx

  • Added a useEffect hook to sync the local page state with the currentPage prop when it changes externally (e.g., from URL navigation)

web/components/templates/requests/RequestsPage.tsx

  • Modified the initial request loading effect to use a minimum drawer size of 33 when opening from URL, ensuring the panel is always visible

Test plan

  • Navigate to /requests?page=5 - should load page 5 directly without resetting to page 1
  • Navigate to /requests?requestId=<valid-request-id> - should auto-open the request detail panel on the right side
  • Combine both: /requests?page=2&requestId=<valid-request-id> - should load page 2 AND open the request detail panel
  • Verify pagination still works correctly when clicking pagination buttons

🤖 Generated with Claude Code

…ail panel

- Fix tableFooter.tsx to sync local page state with currentPage prop when it
  changes externally (e.g., from URL navigation)
- Fix RequestsPage.tsx to ensure drawer opens with a minimum size of 33 when
  requestId is in URL, preventing cases where drawerSize was 0 from localStorage

Fixes ENG-3785

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@vercel
Copy link

vercel bot commented Jan 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
helicone Ready Ready Preview, Comment Jan 9, 2026 0:41am
helicone-eu Ready Ready Preview, Comment Jan 9, 2026 0:41am
1 Skipped Deployment
Project Deployment Review Updated (UTC)
helicone-bifrost Skipped Skipped Jan 9, 2026 0:41am

@supabase
Copy link

supabase bot commented Jan 9, 2026

This pull request has been ignored for the connected project bolqqmqbrciybnypvklh because there are no changes detected in supbase/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@supabase
Copy link

supabase bot commented Jan 9, 2026

This pull request has been ignored for the connected project lmahfbbnchpworytrrqk because there are no changes detected in .supabase/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@claude
Copy link
Contributor

claude bot commented Jan 9, 2026

Claude finished @replicas-connector[bot]'s task —— View job


PR Review Analysis

Review Tasks:

  • Read and analyze changed files
  • Check for critical bugs or security issues
  • Evaluate code quality and best practices
  • Assess performance implications
  • Provide confidence score and recommendations

Review Summary

Score: 8/10 - Good implementation with minor optimization suggestions

This PR effectively addresses both stated issues with clean, straightforward fixes. The code quality is good and follows React best practices.

Detailed Analysis

Strengths

  1. Correct Problem Resolution

    • Page synchronization fix in tableFooter.tsx:46-50 properly handles external page changes
    • Drawer auto-open fix in RequestsPage.tsx:667 ensures visibility with minimum 33% size
  2. React Best Practices

    • Proper use of useEffect with correct dependencies
    • Clean state synchronization pattern
    • Good separation of concerns
  3. Code Safety

    • No security vulnerabilities identified
    • Defensive coding with fallback values
    • Type-safe implementations

🔍 Areas for Minor Improvement

1. Missing Dependency in useEffect (tableFooter.tsx:46-50)

// Current - missing 'page' dependency
useEffect(() => {
  if (currentPage !== page) {
    setPage(currentPage);
  }
}, [currentPage]); // ❌ Missing 'page'

// Suggested
useEffect(() => {
  if (currentPage !== page) {
    setPage(currentPage);
  }
}, [currentPage, page]); // ✅ Complete dependencies

2. Potential Performance Optimization

The current implementation could cause unnecessary re-renders. Consider this pattern:

// Alternative approach to prevent loops
useEffect(() => {
  setPage(currentPage);
}, [currentPage]);

3. Code Documentation

While the PR description is excellent, consider adding inline comments for the non-obvious logic:

// Ensure drawer is visible when opening from URL with stored size of 0
const sizeToUse = drawerSize > 0 ? drawerSize : 33;

📋 Suggestions Summary:

  • Low Priority: Add missing page dependency to useEffect in tableFooter.tsx:50
  • Optional: Consider simplifying the page sync logic to prevent potential infinite loops
  • Enhancement: Add inline comment explaining the 33% minimum drawer size logic

🚀 Performance Impact

Minimal performance impact. The changes are efficient and don't introduce expensive operations.

🔒 Security Assessment

No security concerns identified. URL parameter handling is safe and doesn't introduce vulnerabilities.


Recommendation: Approve and merge. The fixes correctly address the stated issues with clean implementations. The suggested improvements are minor and can be addressed in future iterations if desired.


Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This PR fixes two issues with the requests page URL parameters:

  1. Page parameter now correctly loads - Added a useEffect in tableFooter.tsx to sync the local page state with the currentPage prop when it changes externally (e.g., from URL navigation). This ensures that navigating to /requests?page=5 loads page 5 directly.

  2. Request detail panel auto-opens from URL - Modified the initial request loading logic in RequestsPage.tsx to use a minimum drawer size of 33% when opening from URL, ensuring the panel is visible even if the stored drawerSize is 0.

Critical Issue Found

While the PR correctly fixes the URL-based drawer opening, it misses the same fix for the manual row click scenario. The onExpand callback (line 866-868) still uses the stored drawerSize without the minimum size check. This means if a first-time user (with drawerSize=0 in localStorage) clicks a row to view details, the drawer will "expand" to 0% and remain invisible.

How the Changes Integrate

The changes work within the existing state management pattern:

  • RequestsPage maintains the page state and syncs it with URL query parameters
  • TableFooter receives currentPage as a prop and manages its own local state with debouncing
  • The new useEffect in TableFooter ensures external page changes (URL navigation) properly update the local state
  • The drawer sizing logic uses localStorage to persist user preferences, with the new minimum size check preventing the invisible drawer issue

The page navigation fix is solid, but the drawer expansion needs the same minimum size logic applied to the onExpand callback to fully resolve the issue.

Confidence Score: 2/5

  • This PR partially solves the stated problems but contains a critical bug that will affect user experience
  • Score of 2 reflects that while the page navigation fix is correct and the URL-based drawer opening works, there's a critical logic bug in the onExpand callback that will cause the drawer to be invisible when users click rows if drawerSize is 0 in localStorage. The fix was applied in one location but not the other, creating an inconsistent user experience.
  • web/components/templates/requests/RequestsPage.tsx requires attention - the onExpand callback needs the same minimum size logic that was added to the URL-based opening

Important Files Changed

File Analysis

Filename Score Overview
web/components/templates/requests/RequestsPage.tsx 2/5 Added minimum drawer size check for URL-based opening, but missing same fix for manual row click expansion (critical bug)
web/components/templates/requests/tableFooter.tsx 5/5 Added useEffect to sync page state from prop, correctly handles URL-based page navigation

Sequence Diagram

sequenceDiagram
    participant User
    participant Browser
    participant RequestsPage
    participant TableFooter
    participant ResizablePanel
    participant LocalStorage

    Note over User,LocalStorage: Scenario 1: URL Navigation with ?page=5

    User->>Browser: Navigate to /requests?page=5
    Browser->>RequestsPage: Load with currentPage=5
    RequestsPage->>RequestsPage: useEffect detects router.query.page=5
    RequestsPage->>RequestsPage: setPage(5)
    RequestsPage->>TableFooter: Pass currentPage=5
    TableFooter->>TableFooter: useState initializes with page=5
    TableFooter->>TableFooter: useEffect: currentPage=5 matches page=5
    Note over TableFooter: No sync needed, page loads correctly ✅

    Note over User,LocalStorage: Scenario 2: URL with ?requestId=abc (First Time)

    User->>Browser: Navigate to /requests?requestId=abc
    Browser->>RequestsPage: Load with initialRequestId=abc
    RequestsPage->>LocalStorage: Read drawerSize (returns 0)
    RequestsPage->>RequestsPage: useEffect: initialRequest loaded
    RequestsPage->>RequestsPage: Calculate sizeToUse = max(drawerSize, 33)
    RequestsPage->>ResizablePanel: expand()
    RequestsPage->>ResizablePanel: resize(33)
    ResizablePanel->>RequestsPage: onResize(33)
    RequestsPage->>LocalStorage: Save drawerSize=33
    Note over ResizablePanel: Drawer visible at 33% ✅

    Note over User,LocalStorage: Scenario 3: Click Row (drawerSize=0 in localStorage)

    User->>RequestsPage: Click a row
    RequestsPage->>RequestsPage: onRowSelectHandler
    RequestsPage->>LocalStorage: Read drawerSize (returns 0)
    RequestsPage->>ResizablePanel: expand()
    ResizablePanel->>RequestsPage: onExpand callback
    RequestsPage->>ResizablePanel: resize(drawerSize=0)
    Note over ResizablePanel: Drawer invisible! ❌ BUG

    Note over User,LocalStorage: Scenario 4: User Types Page Number

    User->>TableFooter: Type "7" in page input
    TableFooter->>TableFooter: setPage(7)
    Note over TableFooter: Wait 1200ms for debounce
    TableFooter->>RequestsPage: onPageChange(7)
    RequestsPage->>Browser: router.push with page=7 (shallow)
    Browser->>RequestsPage: router.query.page=7
    RequestsPage->>RequestsPage: useEffect: setPage(7)
    RequestsPage->>TableFooter: currentPage=7
    TableFooter->>TableFooter: useEffect: 7 === 7, no update
    Note over TableFooter: Page navigation works correctly ✅
Loading

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 9, 2026

Additional Comments (1)

web/components/templates/requests/RequestsPage.tsx
The onExpand callback still uses the stored drawerSize without the minimum size check. This creates a bug when a user clicks a row to expand the drawer (line 587 calls expand()), and drawerSize is 0 from localStorage.

Scenario to reproduce:

  1. First-time user opens /requests (no localStorage, drawerSize = 0)
  2. User clicks a row to view details
  3. expand() is called, triggering onExpand -> resize(0)
  4. Drawer remains invisible even though it's "expanded"

The fix applied to the initial URL loading (lines 666-668) should also be applied here:

          onExpand={() => {
            const sizeToUse = drawerSize > 0 ? drawerSize : 33;
            drawerRef.current?.resize(sizeToUse);
          }}

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.

0 participants