feat: add project history - #49
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98f8f51799
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds IndexedDB-backed project history with zipped source archives, snapshot listing, restore, duplication, deletion, and renaming. Successful bundles now save history snapshots and track the active project. Editor updates route through a shared bundling helper. The header includes a history button that opens a drawer with snapshot management, new-project actions, loading/error states, and delete confirmation. New drawer, popover, and input UI primitives support the interface. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 2
🧹 Nitpick comments (6)
src/lib/history.ts (2)
66-76: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
createArchivedoesn't clean up the writer on error.Unlike
restoreHistory'stry/finallyaroundzipReader.close()(Line 210-212), ifzipWriter.add()throws partway through the loop,zipWriter.close()is never called. Given zip.js's documented usage always pairsadd/close, consider wrapping this intry/finally(ortry/catch+ rethrow) for consistency and to avoid leaving the writer in an inconsistent state.♻️ Proposed fix
async function createArchive(files: SourceFile[]) { const blobWriter = new BlobWriter("application/zip"); const zipWriter = new ZipWriter(blobWriter); - for (const file of files) { - await zipWriter.add(file.filename, new TextReader(file.text)); - } - - await zipWriter.close(); - return blobWriter.getData(); + try { + for (const file of files) { + await zipWriter.add(file.filename, new TextReader(file.text)); + } + await zipWriter.close(); + return blobWriter.getData(); + } catch (error) { + await zipWriter.close().catch(() => {}); + throw error; + } }🤖 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 `@src/lib/history.ts` around lines 66 - 76, Update createArchive so zipWriter.close() is always invoked when adding an archive entry fails, including errors from zipWriter.add() inside the files loop. Wrap the add-and-close flow in try/finally, preserve error propagation, and only return blobWriter.getData() after successful closure.
100-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding tests for the concurrent-save / dedup logic.
saveHistory'spendingProjectCreationcoalescing and the update-vs-create fallback (Lines 128-149) are the most concurrency-sensitive part of this new persistence layer, and interact subtly withbundler.ts's request-staleness guards (see the comment there about a delete race). This logic would benefit from unit tests (e.g., fake-indexeddb + concurrentsaveHistorycalls) to lock in the intended behavior.🤖 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 `@src/lib/history.ts` around lines 100 - 186, Add unit tests for saveHistory covering concurrent calls with a null projectId to verify pendingProjectCreation coalesces creation and both callers receive the same snapshot, plus the existing-record update path and update-vs-create fallback when database.history.update reports no rows. Use the project’s IndexedDB test setup or fake-indexeddb, and include the relevant cleanup/reset of pending state between tests.src/store/bundler.ts (1)
93-136: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win"Bundling…" indicator now also covers history archiving/persistence.
isBundlingAtom/bindingLoadingAtomaren't cleared until the outerfinally(Lines 131-136), which now runs after the new archive-zip +saveHistorywork (Lines 105-119) completes. For larger projects this extends how long the header shows "Bundling…" past when the actual bundle output is ready, since persistence is unrelated to the user-visible result. Consider clearing the bundling/loading flags right afterset(bundleResultAtom, result)and letting history persistence happen in the background.🤖 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 `@src/store/bundler.ts` around lines 93 - 136, Update the success path in the bundling flow after set(bundleResultAtom, result) to clear bindingLoadingAtom and isBundlingAtom immediately once the bundle result is available, guarding with isLatestRequest(). Keep saveHistory persistence running afterward without keeping the user-visible bundling indicators active, and prevent the outer finally from redundantly controlling those flags for this completed request.src/components/ui/drawer.tsx (2)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate class token.
data-[state=closed]:animate-outappears twice in the same string. Harmless but redundant.🤖 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 `@src/components/ui/drawer.tsx` at line 49, Remove the duplicate data-[state=closed]:animate-out token from the Drawer component’s class string, preserving all other classes and state-based styling unchanged.
46-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDirection-agnostic slide animation hardcodes "right".
DrawerContent's base classes always applydata-[state=closed]:slide-out-to-right/data-[state=open]:slide-in-from-right, but the direction-specific position classes below correctly branch ondata-[vaul-drawer-direction=...]. Sincevaul'sdirectionprop supportstop/bottom/left/right(defaultbottom), any consumer of this shared primitive that opens from a direction other thanrightwill get content sliding in/out from the wrong side, similar to a known issue in the shadcn-svelte drawer where hardcoded directional classes broke non-bottom drawers. Currently onlydirection="right"is used (HistoryDrawer.tsx), so this is latent, but it will break the moment another direction is added.♻️ Make the slide animation direction-aware
className={cn( - "bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:slide-out-to-right data-[state=open]:slide-in-from-right fixed z-50 flex h-auto flex-col border shadow-lg duration-200", + "bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 fixed z-50 flex h-auto flex-col border shadow-lg duration-200", + "data-[vaul-drawer-direction=right]:data-[state=closed]:slide-out-to-right data-[vaul-drawer-direction=right]:data-[state=open]:slide-in-from-right", + "data-[vaul-drawer-direction=left]:data-[state=closed]:slide-out-to-left data-[vaul-drawer-direction=left]:data-[state=open]:slide-in-from-left", + "data-[vaul-drawer-direction=top]:data-[state=closed]:slide-out-to-top data-[vaul-drawer-direction=top]:data-[state=open]:slide-in-from-top", + "data-[vaul-drawer-direction=bottom]:data-[state=closed]:slide-out-to-bottom data-[vaul-drawer-direction=bottom]:data-[state=open]:slide-in-from-bottom", "data-[vaul-drawer-direction=bottom]:inset-x-0 data-[vaul-drawer-direction=bottom]:bottom-0 data-[vaul-drawer-direction=bottom]:mt-24 data-[vaul-drawer-direction=bottom]:max-h-[80vh] data-[vaul-drawer-direction=bottom]:rounded-t-lg data-[vaul-drawer-direction=bottom]:border-t",🤖 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 `@src/components/ui/drawer.tsx` around lines 46 - 59, Update DrawerContent’s direction-specific animation classes so open and closed slide transitions match the active data-[vaul-drawer-direction] value for top, bottom, left, and right, rather than always using the rightward animation. Preserve the existing direction-specific positioning and fade behavior, and ensure the default bottom direction slides vertically.src/components/HistoryDrawer.tsx (1)
226-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnnecessary Fragment.
The
<>...</>wraps a single<Drawer>child; it can be dropped.Also applies to: 460-462
🤖 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 `@src/components/HistoryDrawer.tsx` around lines 226 - 228, Remove the unnecessary fragment wrapping the single Drawer element in the HistoryDrawer render at the shown location, and apply the same simplification to the corresponding fragment around lines 460-462. Preserve the Drawer structure and behavior unchanged.
🤖 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 `@src/components/HistoryDrawer.tsx`:
- Around line 393-404: Update the Delete trigger and Restore button disabled
conditions in the HistoryDrawer component to also check isStartingNewProject,
matching the existing Copy and Edit button behavior. Preserve the current busyId
checks while preventing either action during handleStartNewProject.
In `@src/store/bundler.ts`:
- Around line 105-119: Before calling saveHistory in the bundle save flow,
re-read currentProjectIdAtom and skip the save when its live value no longer
matches the request’s expected project ID, including when the captured projectId
was non-null and the project was deleted. Update the logic around
projectIdForSave and saveHistory so only a still-valid project request is
persisted; preserve the existing snapshot activation and error handling for
valid saves.
---
Nitpick comments:
In `@src/components/HistoryDrawer.tsx`:
- Around line 226-228: Remove the unnecessary fragment wrapping the single
Drawer element in the HistoryDrawer render at the shown location, and apply the
same simplification to the corresponding fragment around lines 460-462. Preserve
the Drawer structure and behavior unchanged.
In `@src/components/ui/drawer.tsx`:
- Line 49: Remove the duplicate data-[state=closed]:animate-out token from the
Drawer component’s class string, preserving all other classes and state-based
styling unchanged.
- Around line 46-59: Update DrawerContent’s direction-specific animation classes
so open and closed slide transitions match the active
data-[vaul-drawer-direction] value for top, bottom, left, and right, rather than
always using the rightward animation. Preserve the existing direction-specific
positioning and fade behavior, and ensure the default bottom direction slides
vertically.
In `@src/lib/history.ts`:
- Around line 66-76: Update createArchive so zipWriter.close() is always invoked
when adding an archive entry fails, including errors from zipWriter.add() inside
the files loop. Wrap the add-and-close flow in try/finally, preserve error
propagation, and only return blobWriter.getData() after successful closure.
- Around line 100-186: Add unit tests for saveHistory covering concurrent calls
with a null projectId to verify pendingProjectCreation coalesces creation and
both callers receive the same snapshot, plus the existing-record update path and
update-vs-create fallback when database.history.update reports no rows. Use the
project’s IndexedDB test setup or fake-indexeddb, and include the relevant
cleanup/reset of pending state between tests.
In `@src/store/bundler.ts`:
- Around line 93-136: Update the success path in the bundling flow after
set(bundleResultAtom, result) to clear bindingLoadingAtom and isBundlingAtom
immediately once the bundle result is available, guarding with
isLatestRequest(). Keep saveHistory persistence running afterward without
keeping the user-visible bundling indicators active, and prevent the outer
finally from redundantly controlling those flags for this completed request.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 172c7894-b8e6-4f17-a7f1-017798a52226
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.jsonsrc/components/Editor/index.tsxsrc/components/Header.tsxsrc/components/HistoryDrawer.tsxsrc/components/ui/drawer.tsxsrc/components/ui/input.tsxsrc/components/ui/popover.tsxsrc/lib/history.tssrc/store/bundler.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e88d6f76a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| const currentProjectIdStateAtom = atom<number | null>(getStoredCurrentProjectId()); |
There was a problem hiding this comment.
After a project has been saved, reloading the page or opening another same-origin tab initializes this atom from localStorage, while inputFilesAtom is initialized independently from the URL hash or the default preset. The editor's automatic initial bundle then passes the persisted ID to saveHistory, overwriting that project's only archive with unrelated or stale files before the user can restore it. Either restore the stored project's files/version before the initial bundle or keep the active ID session-local/clear it at startup.
Useful? React with 👍 / 👎.
1 participant
No description provided.