the default opened group in the tab list page must be the first one that is not pinned. - #33
Merged
Conversation
Introduce a savedTabGroupMetadata storage key that records pinned tab-group state outside the saved tab payload arrays. This keeps the existing savedTabsIndex and savedTabs:<groupKey> schema backwards compatible while giving the UI a lightweight metadata record to load alongside the index. Normalize metadata defensively so only supported pinned flags survive, and filter metadata whenever groups are replaced or deleted so stale pins cannot outlive their groups. Add a dedicated writeSavedGroupPinned helper so callers can toggle a group pin without loading or rewriting the group's tab list. Focused verification before commit: pnpm exec vitest run tests/unit/storage_utils.test.ts tests/integration/storage.test.ts; pnpm compile; pnpm lint; pnpm smoke.
Add a per-group pin action to the saved-tabs list. Pinned groups are marked in the card UI, expose aria-pressed state, and sort before unpinned groups while preserving the existing newest-first ordering inside each bucket. The pin toggle writes only the lightweight metadata record, so it does not load or rewrite large group tab payloads.
Extend JSON import/export to include groupMetadata alongside savedTabs. Imports remain backwards compatible with old arrays, direct group maps, and { savedTabs } wrappers; metadata is optional and pruned to groups that actually import. Append imports merge pinned flags with existing metadata, while replace imports use only the imported metadata.
Carry pinned group metadata through Drive backup and restore. Backups serialize groupMetadata without settings or install IDs, replace restores write the imported metadata, and merge restores combine incoming pin flags with existing pin flags while still using the existing duplicate-policy merge path.
Focused verification before commit: pnpm exec vitest run tests/unit/list_logic.test.ts tests/integration/actions.test.ts tests/unit/drive_backup_utils.test.ts tests/integration/drive_backup.test.ts tests/unit/storage_utils.test.ts tests/integration/storage.test.ts; pnpm compile; pnpm lint; pnpm smoke.
Update README and storage documentation to describe savedTabGroupMetadata as the lightweight persisted state for pinned groups. The docs now call out that pin toggles avoid rewriting savedTabs:<groupKey> payloads and that the storage permission covers the metadata key. Update the Drive backup spec to show groupMetadata in backup JSON and clarify that it is portable saved-list state, unlike local settings and install IDs. Verification before commit: pnpm lint; pnpm smoke; git diff --check.
Add focused integration coverage for the pinned tab-group metadata storage paths added by the pinning feature. The tests now verify unpinning deletes the compact metadata entry, missing groups cannot receive pin metadata, metadata read failures return an empty safe default, and pin metadata write failures return false without throwing. This restores the branch coverage and line coverage above the main-branch baseline while keeping the tests in the existing integration test folder and leaving production code untouched. Verification run before this commit: pnpm exec vitest run tests/integration/storage.test.ts tests/unit/storage_utils.test.ts; pnpm compile; pnpm lint; pnpm smoke; pnpm test; pnpm test:e2e. Coverage comparison versus main: statements +0.06, branches +0.33, functions +0.04, lines +0.06.
Shrink the shared SVG glyph size used by nufftabs list-page icon buttons from 24px to 20.4px, which is 85% of the previous size and matches the requested roughly 15% reduction. The 48px circular button target remains unchanged so pointer/touch accessibility and layout spacing stay stable; only the visible icon glyph gets smaller. Verification run before commit: pnpm lint; pnpm smoke; pnpm compile.
Replace the aggregate pinned-group metadata write path with per-group metadata keys of the form savedTabGroupMetadata:<groupKey>. Pin toggles now write only the target group's metadata key, which avoids the read-modify-write race where two open list pages could overwrite each other's pin state through the shared metadata map.
Keep reads backward compatible by still loading the legacy savedTabGroupMetadata aggregate map, then overlaying the per-group keys. Unpin writes use an explicit { pinned: false } tombstone so an old aggregate-map pin cannot reappear during migration. Bulk group rewrites without explicit metadata no longer read or rewrite metadata, and removed groups clean up their per-group metadata keys.
Update the list page storage-change listener to refresh on per-group metadata key changes, so multiple open list pages stay in sync after a pin or unpin. Export/import and Drive backup JSON keep the existing groupMetadata object shape; the storage layer translates that portable shape into per-group storage keys.
Update focused storage, list-action, and Drive backup tests to assert the new physical key layout and legacy fallback behavior. Documentation now describes the per-group storage layout, tombstone behavior, and stable backup/export JSON shape.
Verification run before commit: pnpm exec vitest run tests/integration/storage.test.ts tests/unit/storage_utils.test.ts tests/integration/actions.test.ts tests/integration/drive_backup.test.ts tests/unit/drive_backup_utils.test.ts tests/unit/list_logic.test.ts; pnpm compile; pnpm lint; pnpm smoke; git diff --check.
Stop using savedTabsIndex as the read-side source of truth for saved tab groups. readSavedGroupsIndex now enumerates physical savedTabs:<groupKey> storage entries with chrome.storage.local.getKeys(), then normalizes those keys before callers load payloads or metadata. Keep savedTabsIndex as a compatibility mirror for older tooling, fallback reads, and existing debugging flows. Single-group writes, append writes, and bulk writes still update the mirror, but stale mirror writes can no longer hide unrelated groups after concurrent add/delete operations. Add deterministic storage tests for stale index reads, concurrent group adds, concurrent delete/add interleavings, and older runtimes without getKeys. Update the Chrome mock with getKeys support and adjust the Drive restore failure test to fail the write path directly now that group discovery no longer depends on storage.get for savedTabsIndex. Document the key-first storage model in README.md and docs/storage.md, including the performance tradeoff: reads stay linear in the number of storage keys/group entries and avoid loading tab payloads until needed. Verification: pnpm compile; pnpm lint; pnpm smoke; pnpm exec vitest run tests/integration/storage.test.ts tests/unit/drive_backup_utils.test.ts; pnpm exec vitest run tests/integration/storage.test.ts tests/unit/storage_utils.test.ts tests/integration/actions.test.ts tests/integration/drive_backup.test.ts tests/unit/drive_backup_utils.test.ts tests/unit/list_logic.test.ts tests/integration/background_condense.test.ts tests/integration/list_page.test.ts; pnpm test (24 files, 200 tests, coverage 99.06% statements/lines and 90.74% branches); pnpm test:e2e (6 passed).
Previously the function read the saved-groups index to guard against writing metadata for a key that doesn't exist in the index, then wrote the per-group metadata key. That read-before-write pattern introduced a TOCTOU window: if the group was deleted between the index read and the storage write, an orphaned metadata entry would be created. More importantly, the reviewer-flagged concern was about non-atomic read-modify-write patterns; although this function already moved away from the aggregate-map pattern (it writes to an isolated per-group key), the preceding index read still constituted unnecessary state coupling. The fix removes the index read entirely. The write is now a single chrome.storage.local.set call, which is atomic for a single key and races with nothing. Why writing metadata for a non-existent group is safe: - readSavedGroupMetadata already calls filterSavedGroupMetadataForKeys, which cross-references the live index and silently drops any key not present in it. - Every code path that deletes a group (writeSavedGroup with empty tabs, writeAllGroupsInternal) explicitly removes the per-group metadata key via chrome.storage.local.remove, so orphaned entries are short-lived. The test that previously asserted `false` for a missing group key is updated to assert `true` (the write succeeds) and adds a follow-up readSavedGroupMetadata assertion to confirm the orphaned entry is transparently filtered from the read result. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
compareGroupKeys was used as the comparator for Array.prototype.sort.
It called parseGroupCreationTime (string split + Number parse) and
isGroupPinned (object property access) on every comparison, meaning
both ran O(N log N) times for a list of N groups.
The new implementation uses a Schwartzian transform (also called
decorate-sort-undecorate):
1. One O(N) pass maps each key to { key, pinned, createdAt }, calling
parseGroupCreationTime and isGroupPinned exactly once per group.
2. The sort comparator is now O(1): it only does arithmetic and an
optional string comparison, with no string splitting or parsing.
3. A final O(N) map extracts the sorted keys.
Total: O(N) + O(N log N) + O(N) = O(N log N), same asymptotic class but
with a much smaller constant for the O(log N) factor.
Space: O(N) for the intermediate keysWithMeta array. The previous
implementation also allocated an O(N) array via .slice(), so this is
unchanged.
compareGroupKeys is removed because it is no longer called anywhere.
Its logic is inlined into rebuildSortedIndexedGroupKeys, which is the
only place the sort order is computed. The local `type KeyMeta` keeps
the shape of the decorated elements explicit without exposing it outside
the function.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hat is not pinned.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the applyDefaultCollapse function to change the initial state of tab groups. Pinned groups are now collapsed by default, and only the first non-pinned group remains expanded. A review comment correctly identifies that the current early return logic prevents a single pinned group from being collapsed, which contradicts the intended behavior.
When there was exactly one group and it was pinned, the <= 1 early return prevented the collapse logic from running, leaving it expanded. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
=====================================
Coverage 95.3% 95.3%
=====================================
Files 19 19
Lines 2352 2352
Branches 600 600
=====================================
Hits 2242 2242
Misses 98 98
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.