feat(desktop): implement router history with forward/back navigation#1131
Conversation
Replace createHashHistory with a custom persistent history that stores entries in localStorage and syncs the URL hash. Adds back/forward navigation buttons and a recently viewed dropdown to the top bar.
📝 WalkthroughWalkthroughReplaces hash-based history with a new persistentHistory (localStorage-backed) router, adds navigation UI (back/forward buttons + recently-viewed dropdown), registers navigation hotkeys, and wires persistence and UI to the router/history API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant TopBar
participant Router as TanStackRouter
participant persistentHistory as persistentHistory
participant LocalStorage as localStorage
rect rgba(220,235,255,0.5)
User->>TopBar: click Back / Forward / History item
TopBar->>Router: call router.navigate (or router.history.back()/forward())
Router->>persistentHistory: invoke back/forward/push/replace
persistentHistory->>LocalStorage: persist updated entries + index
persistentHistory->>Router: update current location (hash/url)
Router->>TopBar: update location/state (re-render)
TopBar->>User: render updated route
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.ts`:
- Around line 117-122: When restoring persisted history, don't overwrite
original timestamps with Date.now(); instead read persisted.timestamps (falling
back to Date.now() for missing entries) and use those values when building the
states array. Update the load path that currently creates timestamps: replace
the entries.map(() => Date.now()) logic with a mapping that pulls
persisted.timestamps[i] || Date.now(), and propagate that timestamp into the
LocationState object (either by adding a timestamp argument to assignKeyAndIndex
or by setting the timestamp on each state after assignKeyAndIndex). Also ensure
the save path persists timestamps alongside persisted.entries so future reloads
can restore them.
- Around line 86-108: Add unit tests for the parseHref function in
persistent-hash-history.ts to validate parsing across cases: no search/hash,
search only, hash only, search before hash, hash before search, and empty
query/hash segments; assert returned HistoryLocation fields (href, pathname,
search, hash, state) match expected values for each input and include at least
one test using a non-root pathname (e.g., '/workspace/abc') and one with both
query and fragment to ensure the substring logic (searchIndex/hashIndex and
pathname slicing) in parseHref behaves correctly and prevents regressions.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/NavigationControls/components/HistoryDropdown/hooks/useRecentlyViewed/useRecentlyViewed.ts (1)
35-37: Consider extracting default limit to a named constant.The default limit of
20is a magic number. Per coding guidelines, consider extracting it to a module-level constant for clarity and easier maintenance.♻️ Suggested refactor
+const DEFAULT_RECENTLY_VIEWED_LIMIT = 20; + -export function useRecentlyViewed(limit = 20): RecentlyViewedEntry[] { +export function useRecentlyViewed(limit = DEFAULT_RECENTLY_VIEWED_LIMIT): RecentlyViewedEntry[] {apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/NavigationControls/components/HistoryDropdown/HistoryDropdown.tsx (1)
110-117: Type assertion onstatusTypecould cause runtime issues.The
task.statusType as StatusTypeassertion bypasses type checking. If the database contains an unexpected status type, this could cause rendering issues or crashes inStatusIcon.Consider validating the type or using a type guard before rendering:
♻️ Suggested approach
// Option 1: Add a fallback for unknown types const validStatusTypes = ["todo", "in_progress", "done", /* ... other valid types */] as const; const safeStatusType = validStatusTypes.includes(task.statusType as StatusType) ? (task.statusType as StatusType) : "todo"; // fallback // Option 2: If StatusIcon already handles unknown types gracefully, document this assumptionapps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.ts (1)
40-42: Silent error swallowing in localStorage operations.Per coding guidelines, errors should at minimum be logged. While localStorage failures are non-critical, logging helps with debugging:
♻️ Suggested improvement
} catch { - // Corrupted or unavailable — fall through to default + console.warn("[persistent-hash-history/load] Failed to load persisted state, using default"); } } catch { - // localStorage full or unavailable — ignore + console.warn("[persistent-hash-history/persist] Failed to persist state"); }As per coding guidelines: "Never silently swallow errors with
catch(() => {})orcatch(e) { return null }" and "Log errors at minimum if not re-throwing".Also applies to: 60-62
apps/desktop/src/renderer/lib/persistent-hash-history/index.ts (1)
1-1: Consider exporting additional types for consumers.The barrel only exports
persistentHistory, butHistoryEntryandPersistentHashHistorytypes might be useful for consumers who need type-safe access togetEntries()results.♻️ Suggested addition
-export { persistentHistory } from "./persistent-hash-history"; +export { + type HistoryEntry, + type PersistentHashHistory, + persistentHistory, +} from "./persistent-hash-history";apps/desktop/src/renderer/index.tsx (1)
5-7: Consider using therenderer/alias for consistency.Other files in this PR use the
renderer/alias (e.g.,renderer/lib/persistent-hash-history), but this file uses a relative import. For consistency:♻️ Suggested change
-import { persistentHistory } from "./lib/persistent-hash-history"; +import { persistentHistory } from "renderer/lib/persistent-hash-history";As per coding guidelines: "Use alias as defined in
tsconfig.jsonwhen possible".apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/NavigationControls/NavigationControls.tsx (1)
12-13: AddcanGoForward()method to PersistentHashHistory for consistency withcanGoBack().The custom history implementation exposes
canGoBack()as a dedicated API method, butcanGoForwardin NavigationControls relies on manually accessinglocation.state.__TSR_index. While__TSR_indexis intentionally set in the persistent-hash-history implementation and the calculation is correct, this creates an asymmetry: one capability uses the public API while the other accesses state directly.Adding a
canGoForward()method toPersistentHashHistorywould:
- Maintain API consistency with
canGoBack()- Reduce direct state access in UI components
- Make the logic testable alongside other history methods
| function parseHref(href: string, state: LocationState): HistoryLocation { | ||
| const searchIndex = href.indexOf("?"); | ||
| const hashIndex = href.indexOf("#"); | ||
| return { | ||
| href, | ||
| pathname: href.substring( | ||
| 0, | ||
| hashIndex > 0 | ||
| ? searchIndex > 0 | ||
| ? Math.min(hashIndex, searchIndex) | ||
| : hashIndex | ||
| : searchIndex > 0 | ||
| ? searchIndex | ||
| : href.length, | ||
| ), | ||
| hash: hashIndex > -1 ? href.substring(hashIndex) : "", | ||
| search: | ||
| searchIndex > -1 | ||
| ? href.slice(searchIndex, hashIndex === -1 ? undefined : hashIndex) | ||
| : "", | ||
| state, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file and explore the persistent-hash-history module
fd -e ts -e tsx "persistent-hash-history" apps/desktop/src/renderer/lib/Repository: superset-sh/superset
Length of output: 230
🏁 Script executed:
# Check test files for this module
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | grep -i "persistent-hash-history\|history"Repository: superset-sh/superset
Length of output: 149
🏁 Script executed:
# Search for parseHref usage and tests
rg -n "parseHref" --type ts --type tsx apps/Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the test file to check edge cases
cat -n apps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.test.tsRepository: superset-sh/superset
Length of output: 9352
🏁 Script executed:
#!/bin/bash
# Read the full implementation file for context
cat -n apps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.tsRepository: superset-sh/superset
Length of output: 5964
Add unit tests for parseHref to ensure correct parsing of URLs with various components.
The function correctly handles empty search and hash strings, but lacks dedicated test coverage. While the logic handles URLs with query and hash in any order, the function is only used with simple paths (e.g., /tasks, /workspace/abc) in practice. However, explicit tests would clarify the expected behavior and prevent regressions if the usage pattern changes.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.ts`
around lines 86 - 108, Add unit tests for the parseHref function in
persistent-hash-history.ts to validate parsing across cases: no search/hash,
search only, hash only, search before hash, hash before search, and empty
query/hash segments; assert returned HistoryLocation fields (href, pathname,
search, hash, state) match expected values for each input and include at least
one test using a non-root pathname (e.g., '/workspace/abc') and one with both
query and fragment to ensure the substring logic (searchIndex/hashIndex and
pathname slicing) in parseHref behaves correctly and prevents regressions.
| const entries: string[] = [...persisted.entries]; | ||
| const timestamps: number[] = entries.map(() => Date.now()); | ||
| const states: LocationState[] = entries.map((_entry, i) => | ||
| assignKeyAndIndex(i), | ||
| ); | ||
| let index = persisted.index; |
There was a problem hiding this comment.
Timestamps are lost on app reload.
When loading persisted state, all entries are assigned the current timestamp (Date.now()), losing the original navigation timestamps. This affects the "Recently Viewed" feature's ability to show accurate chronological order after an app restart.
Consider persisting timestamps alongside paths:
🐛 Suggested fix
interface PersistedState {
- entries: string[];
+ entries: Array<{ path: string; timestamp: number }>;
index: number;
}
function loadPersistedState(): PersistedState {
// ... parsing logic updated to handle new structure
}
function persistState(entries: string[], timestamps: number[], index: number) {
- const capped = entries.length > MAX_ENTRIES
- ? entries.slice(entries.length - MAX_ENTRIES)
- : entries;
+ const combined = entries.map((path, i) => ({ path, timestamp: timestamps[i]! }));
+ const capped = combined.length > MAX_ENTRIES
+ ? combined.slice(combined.length - MAX_ENTRIES)
+ : combined;
// ...
}🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.ts`
around lines 117 - 122, When restoring persisted history, don't overwrite
original timestamps with Date.now(); instead read persisted.timestamps (falling
back to Date.now() for missing entries) and use those values when building the
states array. Update the load path that currently creates timestamps: replace
the entries.map(() => Date.now()) logic with a mapping that pulls
persisted.timestamps[i] || Date.now(), and propagate that timestamp into the
LocationState object (either by adding a timestamp argument to assignKeyAndIndex
or by setting the timestamp on each state after assignKeyAndIndex). Also ensure
the save path persists timestamps alongside persisted.entries so future reloads
can restore them.
Remove non-null assertions in persistent-hash-history, use fuzzy route matching for Tasks and workspace sidebar highlighting.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.ts`:
- Around line 22-57: Both loadPersistedState and persistState currently swallow
all exceptions; update their catch blocks to log the caught error with
contextual prefixed messages (e.g. "[persistent-hash-history/load] <message>"
and "[persistent-hash-history/persist] <message>") using console.error and
include the Error object so JSON/localStorage/quota issues are visible; modify
loadPersistedState's catch to console.error("[persistent-hash-history/load]
failed to load persisted state:", err) and persistState's catch to
console.error("[persistent-hash-history/persist] failed to persist state:", err)
while leaving the rest of the logic unchanged.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.ts (1)
43-103: Use params objects for multi-arg helpers.
persistState,assignKeyAndIndex, andparseHrefall take 2+ positional arguments. Please switch these to named params objects for clarity and to avoid future boolean/positional confusion.♻️ Example refactor (apply similarly to these helpers)
-function persistState(entries: string[], index: number) { +function persistState({ + entries, + index, +}: { + entries: string[]; + index: number; +}) { try { @@ - localStorage.setItem( + localStorage.setItem( STORAGE_KEY, JSON.stringify({ entries: capped, index: cappedIndex }), ); } catch {} } -function assignKeyAndIndex( - index: number, - state?: LocationState, -): LocationState { +function assignKeyAndIndex({ + index, + state, +}: { + index: number; + state?: LocationState; +}): LocationState { const key = createRandomKey(); return { ...(state ?? {}), @@ -function parseHref(href: string, state: LocationState): HistoryLocation { +function parseHref({ + href, + state, +}: { + href: string; + state: LocationState; +}): HistoryLocation { const searchIndex = href.indexOf("?"); const hashIndex = href.indexOf("#"); return { href, @@ -const getLocation = () => - parseHref(entries[index] ?? "/", states[index] ?? assignKeyAndIndex(index)); +const getLocation = () => + parseHref({ + href: entries[index] ?? "/", + state: states[index] ?? assignKeyAndIndex({ index }), + }); @@ - persistState(entries, index); + persistState({ entries, index });As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties instead of positional arguments.
| function loadPersistedState(): PersistedState { | ||
| try { | ||
| const raw = localStorage.getItem(STORAGE_KEY); | ||
| if (raw) { | ||
| const parsed = JSON.parse(raw) as PersistedState; | ||
| if ( | ||
| Array.isArray(parsed.entries) && | ||
| parsed.entries.length > 0 && | ||
| typeof parsed.index === "number" | ||
| ) { | ||
| const index = Math.min( | ||
| Math.max(parsed.index, 0), | ||
| parsed.entries.length - 1, | ||
| ); | ||
| return { entries: parsed.entries, index }; | ||
| } | ||
| } | ||
| } catch {} | ||
| return { entries: ["/"], index: 0 }; | ||
| } | ||
|
|
||
| function persistState(entries: string[], index: number) { | ||
| try { | ||
| const capped = | ||
| entries.length > MAX_ENTRIES | ||
| ? entries.slice(entries.length - MAX_ENTRIES) | ||
| : entries; | ||
| const cappedIndex = | ||
| entries.length > MAX_ENTRIES | ||
| ? index - (entries.length - MAX_ENTRIES) | ||
| : index; | ||
| localStorage.setItem( | ||
| STORAGE_KEY, | ||
| JSON.stringify({ entries: capped, index: cappedIndex }), | ||
| ); | ||
| } catch {} |
There was a problem hiding this comment.
Don’t swallow localStorage errors—log with context.
Both loadPersistedState and persistState silently ignore failures, which makes storage issues invisible in production. Please log with the required prefix so we can diagnose storage/quota/JSON failures.
🔧 Suggested fix
function loadPersistedState(): PersistedState {
try {
const raw = localStorage.getItem(STORAGE_KEY);
@@
- } catch {}
+ } catch (error) {
+ console.error(
+ "[router-history/loadPersistedState] failed to read persisted history",
+ error,
+ );
+ }
return { entries: ["/"], index: 0 };
}
function persistState(entries: string[], index: number) {
try {
@@
- } catch {}
+ } catch (error) {
+ console.error(
+ "[router-history/persistState] failed to persist history",
+ error,
+ );
+ }
}As per coding guidelines: Never swallow errors silently; at minimum log them with context; use prefixed console logging with pattern [domain/operation] message for all logging.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.ts`
around lines 22 - 57, Both loadPersistedState and persistState currently swallow
all exceptions; update their catch blocks to log the caught error with
contextual prefixed messages (e.g. "[persistent-hash-history/load] <message>"
and "[persistent-hash-history/persist] <message>") using console.error and
include the Error object so JSON/localStorage/quota issues are visible; modify
loadPersistedState's catch to console.error("[persistent-hash-history/load]
failed to load persisted state:", err) and persistState's catch to
console.error("[persistent-hash-history/persist] failed to persist state:", err)
while leaving the rest of the logic unchanged.
Kitenite
deleted the
implement-router-history-with-forward-back-navigat
branch
1 participant
Summary
createHashHistorywith a custom persistent history (persistent-hash-history) that stores navigation entries in localStorage and syncs the URL hashTest plan
router-historykey)bun test apps/desktop/src/renderer/lib/persistent-hash-history/passes (19 tests)bun run typecheck --filter=@superset/desktoppassesSummary by CodeRabbit
New Features
Behavior Changes