feat(history): implement Phase 1 history improvements with sorting and enhanced search experience#1034
Conversation
👷 Deploy request for docmagic-muneer pending review.Visit the deploys page to approve it
|
👷 Deploy request for docmagic1 pending review.Visit the deploys page to approve it
|
|
@kareena0229 is attempting to deploy a commit to the muneerali199's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe history dashboard now supports URL-synced tabs, sorting, updated history loading, revised preview rendering, and reworked item cards, empty states, pagination, and page-shell presentation. ChangesHistory dashboard changes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@components/dashboard/history-dashboard.tsx`:
- Around line 327-328: The `activeTab` state in `history-dashboard.tsx` is only
initialized from `urlTab` once, so it can drift from the current `?tab=` value
during browser back/forward navigation. Update the `HistoryDashboard` logic so
`activeTab` stays synchronized with the URL by reacting to `searchParams`
changes (for example in the same area as the `useState(urlTab)` setup and the
fetch/tab logic around the referenced section) and keep the tab setter and
URL-reading code aligned. Ensure the tab used for rendering and data fetching
always reflects the latest `searchParams.get("tab")` value rather than stale
state.
- Around line 122-129: The resume branch in getDocumentDescription is only
checking content.resumeData?.name, which causes many resumes to fall back to
"Resume" and breaks search matching for candidate names. Update the resume case
in getDocumentDescription to also read the display name from
resumeData.personal_info / resumeData.personalInfo (and any existing top-level
name fallback already used elsewhere) before defaulting to "Resume", so the
returned description continues to surface the candidate name for history cards
and filtering.
- Around line 525-528: The switch cases in history-dashboard.tsx are missing
block scope around the "generated" and "letter" branches, so the const
declarations inside them can conflict with other cases. Wrap the bodies of the
switch cases handled by the generated and letter logic in curly braces to create
separate scopes, keeping the declarations local to each case in the switch
statement.
- Around line 126-139: The switch in history-dashboard’s document title logic
has case-level const declarations that violate noSwitchDeclarations. Update the
doc.type switch so the presentation and generated cases use block scopes with
braces around the case bodies, keeping the existing getPresentationSlides and
metadata.sections/doc.sections/content.sections logic intact while isolating the
slides and sections variables.
- Around line 360-423: The sorting in history fetching is applied after
pagination, so the current page is always fetched in newest order and then
reordered locally. Update the query in fetchHistory so the selected sort order
is applied on the Supabase request before range(from, to), using the same sortBy
cases already handled in the local filtered sort logic. Keep the local
setFilteredItems sorting in sync with fetchHistory, but ensure the database
query determines the page contents for newest, oldest, and az.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 657f560d-f3e2-428d-b13e-223b9cc15a35
📒 Files selected for processing (1)
components/dashboard/history-dashboard.tsx
Muneerali199
added
gssoc:approved
Muneerali199
left a comment
Muneerali199
left a comment
There was a problem hiding this comment.
Review: Changes requested
Good work on the sorting and search improvements. I found a few issues that need to be addressed before merging.
Blocking: Pagination + client-side sort gives incorrect results
The sort is applied client-side after pagination. The Supabase query always uses order("created_at", { ascending: false }).range(from, to), so page 1 always contains the newest N items. Then the client re-sorts those N items. This means:
- "Oldest First" shows the oldest items from the first page of newest results — not the actual globally oldest items
- "A-Z" sorts only page 1 items alphabetically, missing items on pages 2+
Fix: Apply the sortBy value to the Supabase query before .range(from, to) so pagination reflects the chosen sort order.
Blocking: noSwitchDeclarations — missing block scope in switch cases
Lines 126-139: The case "generated": branch declares const sections without {} block scope. This violates noSwitchDeclarations and will cause a redeclaration error if cases stack.
Fix: Wrap each case body that has const/let declarations in curly braces.
High: activeTab not synced with URL
Line 327: useState(urlTab) only reads the URL on initial mount. Browser back/forward navigation will update the URL but activeTab state will drift.
Fix: Use a useEffect on searchParams.get("tab") to keep activeTab in sync.
Suggestions (non-blocking):
- Add
level:intermediatelabel to match the issue complexity - Consider separating formatting-only changes (quotes, import style) from feature changes in future PRs — it inflates the diff
Please address the blocking items and I'll re-review.
Will update Soon |
|
Thanks for the detailed review! It has been helpful to me. I've addressed the requested changes:
I also verified the changes locally, and all project tests passed successfully ( Please let me know if there are any additional improvements you'd like me to make. Thanks again for the helpful feedback! @Muneerali199 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/dashboard/history-dashboard.tsx (2)
444-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid positional index mapping of
typeCounts.
typeCounts[0..4]assumesObject.keys(contentTypeConfig)enumerates exactly in the orderresume, presentation, generated, diagram, letter. If the config keys are ever reordered or a type is added/removed, the counts silently map to the wrong stat. Build the result by associating each count with its source key instead.♻️ Map counts by type key
- const typeCounts = await Promise.all( - Object.keys(contentTypeConfig).map((type) => - supabase - .from("documents") - .select("id", { count: "exact", head: true }) - .eq("user_id", user.id) - .eq("type", type), - ), - ); - - setStats({ - total: count || 0, - resume: typeCounts[0].count || 0, - presentation: typeCounts[1].count || 0, - generated: typeCounts[2].count || 0, - diagram: typeCounts[3].count || 0, - letter: typeCounts[4].count || 0, - }); + const types = Object.keys(contentTypeConfig) as ContentType[]; + const typeCounts = await Promise.all( + types.map((type) => + supabase + .from("documents") + .select("id", { count: "exact", head: true }) + .eq("user_id", user.id) + .eq("type", type), + ), + ); + const countsByType = Object.fromEntries( + types.map((type, i) => [type, typeCounts[i].count || 0]), + ) as Record<ContentType, number>; + + setStats({ + total: count || 0, + resume: countsByType.resume || 0, + presentation: countsByType.presentation || 0, + generated: countsByType.generated || 0, + diagram: countsByType.diagram || 0, + letter: countsByType.letter || 0, + });🤖 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 `@components/dashboard/history-dashboard.tsx` around lines 444 - 461, The stats aggregation in history-dashboard.tsx is relying on positional access to typeCounts, which can break if contentTypeConfig key order changes. Update the Promise.all mapping around the count query to preserve each type key with its result, then build setStats from a keyed lookup rather than typeCounts[0..4]. Use the existing type names from contentTypeConfig and the stats fields in setStats to ensure each document count is assigned to the correct category.
800-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sortByis not URL-backed unliketab/page/pageSize.Tab and pagination are persisted via
updateQueryParams, but the sort selection lives only in local state. Browser back/forward and refresh won't restore it, which is slightly inconsistent with the URL-backed history-navigation contract. Also consider resettingpageto 1 when the sort changes so users aren't left on an out-of-range page. Optional for Phase 1.🤖 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 `@components/dashboard/history-dashboard.tsx` around lines 800 - 810, The sort selection in history-dashboard’s select control is only stored in local state, unlike tab/page/pageSize which are synced through updateQueryParams. Update the sortBy change handler in HistoryDashboard to write the new sort value into the URL query params (using the same pattern as the existing pagination/tab setters), and also reset page to 1 when the sort changes so navigation stays consistent and avoids out-of-range pages. Use the existing sortBy state and updateQueryParams helper to locate and wire this behavior.
🤖 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 `@components/dashboard/history-dashboard.tsx`:
- Around line 444-461: The stats aggregation in history-dashboard.tsx is relying
on positional access to typeCounts, which can break if contentTypeConfig key
order changes. Update the Promise.all mapping around the count query to preserve
each type key with its result, then build setStats from a keyed lookup rather
than typeCounts[0..4]. Use the existing type names from contentTypeConfig and
the stats fields in setStats to ensure each document count is assigned to the
correct category.
- Around line 800-810: The sort selection in history-dashboard’s select control
is only stored in local state, unlike tab/page/pageSize which are synced through
updateQueryParams. Update the sortBy change handler in HistoryDashboard to write
the new sort value into the URL query params (using the same pattern as the
existing pagination/tab setters), and also reset page to 1 when the sort changes
so navigation stays consistent and avoids out-of-range pages. Use the existing
sortBy state and updateQueryParams helper to locate and wire this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6fcc0eb-57f4-4614-b87d-114093700b76
📒 Files selected for processing (1)
components/dashboard/history-dashboard.tsx
|
@kareena0229 main has advanced with recent merges — please rebase and I'll re-approve. The review is approved pending rebase. |
f768d90 to
5aa0272
Compare
|
Thanks for the update! I've rebased my branch onto the latest I also reran the project's test suite after rebasing, and all tests passed successfully. Please let me know if there's anything else you'd like me to address. Thanks again for your review! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/dashboard/history-dashboard.tsx (1)
444-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMap per-type counts by type name, not array position.
setStatsassumestypeCounts[0..4]correspond to resume/presentation/generated/diagram/letter, which only holds whileObject.keys(contentTypeConfig)stays in that exact order. Reordering or adding a key tocontentTypeConfigwould silently misassign the stat cards. Keying the results by type removes the positional coupling.Suggested refactor
- const typeCounts = await Promise.all( - Object.keys(contentTypeConfig).map((type) => - supabase - .from("documents") - .select("id", { count: "exact", head: true }) - .eq("user_id", user.id) - .eq("type", type), - ), - ); - - setStats({ - total: count || 0, - resume: typeCounts[0].count || 0, - presentation: typeCounts[1].count || 0, - generated: typeCounts[2].count || 0, - diagram: typeCounts[3].count || 0, - letter: typeCounts[4].count || 0, - }); + const types = Object.keys(contentTypeConfig); + const typeCounts = await Promise.all( + types.map((type) => + supabase + .from("documents") + .select("id", { count: "exact", head: true }) + .eq("user_id", user.id) + .eq("type", type), + ), + ); + + const countsByType = Object.fromEntries( + types.map((type, i) => [type, typeCounts[i].count || 0]), + ); + setStats({ + total: count || 0, + resume: countsByType.resume || 0, + presentation: countsByType.presentation || 0, + generated: countsByType.generated || 0, + diagram: countsByType.diagram || 0, + letter: countsByType.letter || 0, + });🤖 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 `@components/dashboard/history-dashboard.tsx` around lines 444 - 461, The per-type document counts in history-dashboard are still mapped by array index, which couples setStats to the order of Object.keys(contentTypeConfig). Update the Promise.all result handling in history-dashboard.tsx to associate each count with its type name from contentTypeConfig, then build setStats from those keyed values instead of typeCounts[0..4]. Use the existing contentTypeConfig keys and the setStats call to locate the refactor.
🤖 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 `@components/dashboard/history-dashboard.tsx`:
- Around line 444-461: The per-type document counts in history-dashboard are
still mapped by array index, which couples setStats to the order of
Object.keys(contentTypeConfig). Update the Promise.all result handling in
history-dashboard.tsx to associate each count with its type name from
contentTypeConfig, then build setStats from those keyed values instead of
typeCounts[0..4]. Use the existing contentTypeConfig keys and the setStats call
to locate the refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c58d5db-7cae-4bf4-90df-37a4c7dfaedc
📒 Files selected for processing (1)
components/dashboard/history-dashboard.tsx
2 participants
📌 Related Issue
Closes #1032 (Phase 1)
📝 Overview
This PR introduces the first phase of the AI Generation History improvements by enhancing the existing history browsing experience. Instead of implementing the entire feature in one large PR, this contribution focuses on foundational improvements that are easier to review and extend.
✨ What's Included
Added sorting options for history items:
Integrated sorting with the existing search functionality.
Improved the empty state message when no matching documents are found.
Preserved existing pagination, tabs, and history navigation behavior.
🚀 Benefits
🧪 Testing
npm run dev).📌 Notes
The production build currently reports unrelated project configuration issues (Supabase environment configuration and existing admin module imports) that are outside the scope of this PR and were not introduced by these changes.
🔮 Future Improvements
This PR intentionally focuses on Phase 1.
Planned follow-up enhancements include:
Summary by CodeRabbit