Implement commit messaging and added commit category in settings

[Jordonbc]

No description provided.

[github-actions]

PR Review: Implement commit messaging and added commit category in settings

14 files changed, 547 insertions, 42 deletions

---

[Blocking] Commit button stays disabled when only a hint is available

Frontend/src/scripts/ui/layout.ts:196 and Frontend/src/scripts/features/repo/commit.ts:77-93

refreshRepoActions() calls updateCommitButton() at line 196, then immediately overrides the commit button's disabled state using its own summaryFilled calculation at line 243:

// layout.ts refreshRepoActions()
const summaryFilled = (summary?.value.trim().length ?? 0) > 0;  // line 237 - NO hint check

The new updateCommitButton() in commit.ts correctly considers the hint when computing summaryFilled (line 87: || !!hint). But since refreshRepoActions runs unconditionally after updateCommitButton and uses a different (hint-unaware) definition of summaryFilled, the button is re-disabled.

Impact: When exactly one file is selected and commit_message_template_enabled is true, the placeholder shows the template hint but the commit button stays disabled because no text has been typed. The user cannot commit, defeating the entire prefill feature.

Fix: Either import getCommitSummaryHint into layout.ts and add || !!getCommitSummaryHint() to the summaryFilled check at line 237, or remove the redundant button-disabled assignment from refreshRepoActions and let updateCommitButton be the sole authority.

---

[Important] Config migration silently drops restrict_commit_summary for existing users

Backend/src/settings.rs — restrict_commit_summary moved from General to Commit struct, but no migration path.

Old configs store restrict_commit_summary under [general]. The migrate() function is a no-op and schema_version remains 1. The old field is silently dropped by serde (no #[serde(deny_unknown_fields)]), and commit.restrict_commit_summary gets its default of true.

Impact: Any user who explicitly disabled the 72-char limit (restrict_commit_summary = false) will silently have it re-enabled after updating. Data loss for a user-opted setting.

Fix: Bump schema_version to 2 and add a migration step in migrate() that reads the old restrict_commit_summary from general and writes it into commit. Also add a #[serde(alias = "restrict_commit_summary")] or manual deserialization approach to catch the old field.

---

[Important] Empty template strings silently revert to defaults

Frontend/src/scripts/features/repo/commit.ts:44-49

return globalSettings?.commit?.commit_templates?.commit_message_template_create || DEFAULT_COMMIT_MESSAGE_CREATE;

Uses || (not ??), so an empty string '' stored in settings (user cleared the field) is treated as falsy and the default template is substituted. This makes it impossible for a user to clear a single template category and get no prefill — the only way to suppress prefill is to disable the entire feature with the checkbox.

Meanwhile loadCommitSettingsIntoForm (line 44 of settingsCommit.ts) correctly preserves empty strings via ??, so the round-trip saves empty → loads empty → but the runtime ignores the empty.

Impact: Users who clear a template field expecting no prefill for that category will silently get the default template instead.

Fix: Use ?? instead of || in selectedCommitTemplate(), and either accept empty string as "no prefill for this category" or, if the intent is to always have a non-empty template, prevent saving empty strings at the form-collection level.

---

[Minor] Test coverage gaps

Frontend/src/scripts/features/repo/commit.test.ts

No tests for:

• commit_message_template_enabled: false (hint should be suppressed)
• restrict_commit_summary: false (hint should not be truncated)
• Multiple files selected (hint should be suppressed)
• The {file:path} placeholder expansion
• Status values A, D, ?, status.includes('?') for different template selection paths

opencode session  |  github run

[github-actions]

Review

[Important] Config migration silently drops restrict_commit_summary for existing users

File: Backend/src/settings.rs lines 79–99, 182–202, 813–820

The restrict_commit_summary field was moved from the General struct to the new Commit struct. The migrate() function is a no-op and schema_version remains 1. When an existing config saved with the old layout (where restrict_commit_summary lives under [general]) is loaded, serde silently drops the field (no #[serde(deny_unknown_fields)]), and commit.restrict_commit_summary gets its default of true.

Impact: Every user who explicitly disabled the 72-char summary cap (restrict_commit_summary = false) will silently have it re-enabled after updating.

Fix: Bump schema_version to 2, add a migration step that reads the old general.restrict_commit_summary and writes it into commit.restrict_commit_summary, then removes it from general. Also consider #[serde(alias = "restrict_commit_summary")] on the Commit field as a belt-and-suspenders measure in case a user's config is still on the old format when migrate runs.

---

[Minor] updateCommitButton lacks repoOn/changesOn guard

File: Frontend/src/scripts/features/repo/commit.ts:77–93

The old refreshRepoActions (before the PR) included the repoOn && changesOn gate in its commit.disabled assignment. The new updateCommitButton() only considers summary/hint fill and selection state. While selection state is typically empty without an active repo, the missing gate is a regression in defensive coding: a stale state.selectedFiles entry combined with leftover summary text could briefly leave the button enabled without an active repo.

Impact: Low — refreshRepoActions clears the summary field when !changesOn (layout.ts:250–253), and selection state is usually cleared on repo close. But the guard was intentionally present before and was removed without replacement.

---

[Minor] Test coverage gaps

File: Frontend/src/scripts/features/repo/commit.test.ts

No tests for:

• commit_message_template_enabled: false (hint suppression)
• restrict_commit_summary: false (no truncation)
• Multiple files selected (hint should be suppressed)
• The {file:path} placeholder expansion
• Status values A, D, ?, status.includes('?') through the different template selection branches

Acceptable for a first implementation, but worth noting.

---

The original AI review's finding #1 (commit button overridden by refreshRepoActions) and finding #3 (|| vs ?? in selectedCommitTemplate) were addressed by the "Fix issues" commit (79fb810) — the override line was removed and || was changed to ??. Those issues are no longer present in the final code.

opencode session  |  github run

[github-actions]

Here are my review findings:

---

[Important] Config migration silently drops restrict_commit_summary for existing users

File: Backend/src/settings.rs:813-820 (migrate() is a no-op), Backend/src/settings.rs:182-193 (new Commit struct)

The restrict_commit_summary field was moved from General (old format: [general] restrict_commit_summary = false) to the new Commit struct ([commit] section). The migrate() function is a no-op — schema_version stays at 1, and no migration logic reads the old field location.

When load_or_default() calls toml::from_str::<AppConfig> on a pre-PR config file, serde silently drops the general.restrict_commit_summary key (no #[serde(deny_unknown_fields)] on General), and the Commit struct is not present in the old file. commit.restrict_commit_summary gets its default of true.

Impact: Every user who explicitly set restrict_commit_summary = false will silently have the 72-char cap re-enabled after updating. This is a data-loss regression for a user-opted setting with no visible indication that it was changed.

Confidence: High — the old and new struct layouts are clearly visible, the deserialization path is straightforward, and migrate() demonstrably does not handle this.

Fix: Either (a) bump schema_version to 2 and in migrate() read the old value from the raw TOML before deserialization drops it, or (b) in load_or_default() parse the raw TOML with toml::Table, check if general.restrict_commit_summary exists and [commit] section does not, and transfer the value manually before the full deserialization. Both approaches are valid; (b) is surgical because it only affects the one load path where the old format actually exists on disk.

---

No other blocking or important issues were found. The earlier review's concerns about the commit button being overridden by refreshRepoActions and about || vs ?? in selectedCommitTemplate were addressed in commits 79fb810 and c3a0d2c. The repoOn/changesOn guards in updateCommitButton match the semantics of hasRepo() and hasChanges() respectively (commit.ts:82-83 vs state.ts:86,89-90). The empty-template round-trip (form → || '' in collectCommitTemplateSettings, form load → ?? in loadCommitSettingsIntoForm, runtime → ?? in selectedCommitTemplate) correctly preserves user-cleared template fields as empty.

opencode session  |  github run

[github-actions]

Summary

One blocking finding: the config migration silently drops restrict_commit_summary for existing users who explicitly disabled it, because the field was moved from General to Commit without any migration step or schema version bump. A minor cosmetic indentation inconsistency was also noted in the test file.

opencode session  |  github run

[github-actions]

Review

[Important] Config migration silently drops restrict_commit_summary for existing users

Backend/src/settings.rs:813-820 (migrate() is a no-op), Backend/src/settings.rs:182-193 (new Commit struct)

Confirmed via diff: restrict_commit_summary was moved from General (old [general] section) to the new Commit struct ([commit] section). The old field at General.restrict_commit_summary was deleted (lines 99-101 removed) and a new field at Commit.restrict_commit_summary was added (lines 188-189).

When load_or_default() parses a pre-PR config file:

1. General no longer has restrict_commit_summary, so serde drops it silently (no #[serde(deny_unknown_fields)] on General).
2. No [commit] section exists in the old file, so Commit::default() is used — restrict_commit_summary defaults to true.
3. migrate() matches schema_version: 1 and does nothing.

Impact: Every user who explicitly set restrict_commit_summary = false will silently have the 72-char cap re-enabled after updating. This is a user-opted setting lost without indication.

Confidence: High

Fix: Bump schema_version to 2 and add a migration step in migrate() that reads the old general.restrict_commit_summary from raw TOML and writes it into commit.restrict_commit_summary. Alternatively, parse the file as toml::Table first in load_or_default() and transfer the value before full deserialization.

---

No blocking issues found. The earlier review concerns (layout.ts button override, || vs ?? in selectedCommitTemplate) were correctly addressed in commits 79fb810 and c3a0d2c. The empty-template round-trip (form collect via ||, load via ??, runtime via ??) correctly preserves user-cleared fields as empty strings. Tests cover the core hint logic, template selection by status, truncation, disabled feature, and the {file:path} placeholder. No other important or blocking issues were identified.

opencode session  |  github run

[github-actions]

After reviewing all 19 changed files, I have one finding:

---

[Important] Config migration silently drops restrict_commit_summary for existing users

Backend/src/settings.rs:78-99 (General — field removed), Backend/src/settings.rs:182-203 (Commit — field added), Backend/src/settings.rs:813-820 (migrate() is a no-op)

The restrict_commit_summary boolean was moved from the General struct (old TOML section [general]) to the new Commit struct ([commit]). No migration path exists to carry forward a user's prior setting.

When load_or_default() deserializes a pre-PR config file:

1. General no longer has a restrict_commit_summary field → serde ignores the old key silently (no #[serde(deny_unknown_fields)] on General).
2. No [commit] section exists in the old file → Commit::default() is used, setting restrict_commit_summary = true (via #[serde(default = "default_true")]).
3. migrate() checks schema_version (still 1 in both old and new defaults) and does nothing.

Every user who explicitly set restrict_commit_summary = false will silently have the 72-character cap re-enabled after updating. The setting is lost without any indication.

Confidence: High — the struct layouts are clearly visible, the deserialization path is straightforward, and migrate() demonstrably does not handle this case.

Fix: Either:

• (a) Bump schema_version default to 2 and add a migration step in migrate() that reads the old general.restrict_commit_summary from the raw TOML (pre-deserialization) and writes it into commit.restrict_commit_summary, or
• (b) In load_or_default(), parse the file as toml::Table first, check if general.restrict_commit_summary exists and commit section does not, and transfer the value manually before the full toml::from_str call.

---

No other blocking or important issues were found. Specifically, the concerns raised in prior reviews about the commit button being overridden by layout.ts:refreshRepoActions and about || vs ?? in selectedCommitTemplate were correctly addressed: updateCommitButton() is now the sole authority for button enablement, and ?? is used in the runtime template selection path to preserve user-cleared fields as empty strings.

opencode session  |  github run

[github-actions]

After reviewing all 21 changed files across 8 commits, I have one finding.

---

[Important] Config migration silently drops restrict_commit_summary for existing users

Backend/src/settings.rs: General.restrict_commit_summary deleted (line 99), Commit.restrict_commit_summary added (line 188–189). The migrate() function (line 813–820) is a no-op at schema_version: 1.

When a pre-PR config file (which stores restrict_commit_summary under [general]) is loaded:

1. General no longer has a restrict_commit_summary field → serde silently drops the old key (no #[serde(deny_unknown_fields)] on General).
2. No [commit] section exists in the old file → Commit::default() is used, giving restrict_commit_summary = true.
3. migrate() matches schema_version: 1 and does nothing.

Impact: Every user who explicitly set restrict_commit_summary = false will silently have the 72-character cap re-enabled after updating, with no visible indication that their setting was lost.

Confidence: High

Fix: Bump schema_version to 2 and either:

• (a) Add a migration step in migrate() that reads the old general.restrict_commit_summary value from the raw TOML before deserialization and writes it into commit.restrict_commit_summary.
• (b) In load_or_default(), parse the file as toml::Table first; if general.restrict_commit_summary exists and commit section does not, transfer the value before full toml::from_str.

---

No other blocking or important issues were found. Specifically, the concerns raised in prior reviews (layout.ts button override, || vs ?? in template selection, missing repoOn/changesOn guard) were correctly addressed in commits 79fb810 and c3a0d2c. The empty-template round-trip correctly preserves '' via ??. The {file:path} placeholder, status-branch selection, truncation, and disabled-feature paths are all tested. The update-button spinner fix in update.ts/notify.ts is unrelated to commit messaging but is an improvement to an existing feature and introduces no regressions.

opencode session  |  github run