feat: preserve tab group actions on quick rename

[ngthuongdoan]

No description provided.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[ngthuongdoan]

Fix #553

[sebastienfontaine]

Thanks for this — the diagnosis is correct and the fix is the right shape.

I confirmed the bug is real: rule matching is a first-match rules.find() (src/common/storage.ts:329), so the freshly-unshifted rename rule shadows the broader one, and TabGroupsService.ts:124-127 routes a rule with no group_id straight to ungroupTab, which then actively ejects the tab from its group at :79-83. Inheriting the action fields while keeping only the title as the override is the right call, and it's nicely contained to the single entry point (background.ts:228). Good detail that _getRuleFromUrl skips disabled rules, so a disabled rule's actions correctly aren't inherited.

Verified locally on 1e13030: full suite 220/220 green, vue-tsc clean.

Nothing here is blocking, and there are no security implications (no new permissions, no injection surface, inherited values all come from the user's own stored rules). Two things I'd like addressed, both small:

• the title_matcher / url_matcher nulling is a no-op in production — and the test covering it leans on a mock that diverges from the real _getDefaultRule
• the second new test silently depends on mock state leaking from the previous one

The rest are suggestions and follow-ups, flagged inline.

One manual check I couldn't cover with unit tests: quick-renaming a tab that now inherits protected: true still hits chrome.tabs.reload at the end of handleRenameTab. The tab was already protected before the rename, so this isn't a regression from your change — but it's worth confirming by hand that the reload isn't blocked by a beforeunload dialog.

[sebastienfontaine]

These two assignments can't change anything in production. rule comes from _getDefaultRule, which already returns title_matcher: null and url_matcher: null (src/common/storage.ts:48-49), and the block above never copies the matchers from matchingRule — so both fields are already null by the time we reach here.

They only look meaningful in the new test because the mocked _getDefaultRule returns 'default-title-matcher' / 'default-url-matcher' (test line 432), values the real function never produces. So the assertion covers a transition that cannot occur.

Either drop both lines plus the comment, or keep them as deliberate defence-in-depth and make the test mock mirror the real defaults so the assertion carries weight.

[sebastienfontaine]

These six assignments happen to cover every action field on Tab (src/common/types.ts:11-21) today. Add a seventh action field later and quick-rename will silently resume clobbering it — precisely the class of bug this PR fixes.

Inverting it to "inherit everything, override the title" makes the intent self-maintaining:

if (matchingRule) {
    rule.tab = { ...matchingRule.tab, title: rule.tab.title, title_matcher: null, url_matcher: null };
}

One caveat worth knowing before you take it: a spread also carries over any legacy or unknown keys that happen to exist on stored rule objects. I didn't verify whether rules are sanitised on read/import, so if they aren't, this trades a sync-drift risk for some harmless dead data. Your call which you'd rather have.

[sebastienfontaine]

_getRuleFromUrl calls _getStorageAsync internally (src/common/storage.ts:324), and line 157 calls it again immediately — so each quick rename now performs two full storage reads and two decompressFromUTF16 passes over the entire settings blob.

Renames are user-initiated and infrequent, so the cost isn't alarming. But matchingRule also ends up coming from a different snapshot than tabModifier. Reading once and matching against that single snapshot would be both cheaper and tidier — e.g. by letting _getRuleFromUrl accept pre-loaded settings.

[sebastienfontaine]

Pre-existing behaviour, not introduced by this PR — so not something I'd ask you to fix here. Flagging it only because the new tests now codify it as expected.

Every quick rename unshifts another rule whose url_fragment is the full href. Rename the same tab three times and storage holds three near-identical, always-matching rules; only the first is ever used, and all three appear in the Options list.

Worth noting that SidePanel.vue:81-93 handles the same situation by loading and updating the matching rule in place. Applying that precedent here — replace when url_fragment already equals urlParams.href, otherwise unshift — would stop the growth and make this inheritance logic largely fall out for free, since you'd be editing the rule that already holds the right actions. Probably a good follow-up issue.

[sebastienfontaine]

This test never stubs _getStorageAsync, so it inherits the mockResolvedValue(storedSettings) set by the previous test. vitest.config.ts sets neither clearMocks nor mockReset, and vi.clearAllMocks() clears call records while preserving implementations — I confirmed that holds on Vitest 4.0.18.

I probed what this test actually receives from _getStorageAsync:

• full-file run: ["rename-rule", "pull-request-rule"] — the previous test's storedSettings, whose rules array that test had already mutated
• run in isolation: [] — the base vi.fn() implementation

It passes either way, because it only asserts on secondRenameRule.tab. But the state it exercises depends on test ordering, which will bite whoever reorders or splits this file later. Worth stubbing _getStorageAsync explicitly here — and asserting the _setStorage payload — so the test stands on its own.

[sebastienfontaine]

This asserts on the object the mocked _getDefaultRule handed back, which verifies the mutation but not that the correct thing was persisted. The storedSettings.rules check just below does cover ordering.

Asserting against the _setStorage argument instead would be more direct, and would survive a refactor that stops mutating rule.tab in place (such as the spread suggested over in TabRulesService.ts).