Skip to content

fix: preserve sidebar channel order in JSON exports - #282

Merged
steipete merged 1 commit into
mainfrom
round7/export-sidebar
Oct 1, 2026
Merged

steipete merged 1 commit into
mainfrom
round7/export-sidebar

Conversation

@steipete

@steipete steipete commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Include saved personal channel order in JSON exports from SQLite and PostgreSQL. The roaming feature stores these preferences in user_sidebar_channel_order, but both export table lists omitted it, so an export lost the saved orders and explicit clears.

Add the table to both exporters and extend their shared sidebar-preference contract to check exported data. The regression failed on both databases before the fix and passes on both afterward. Full remote pnpm check and pnpm coverage pass (86.7%); independent P0–P2 review is clean. Documentation and the 0.7.0 changelog are updated.

Follow-up to #260.

@steipete
steipete requested a review from a team as a code owner October 1, 2026 05:14
@clawsweeper

clawsweeper Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Oct 1, 2026
@clawsweeper

clawsweeper Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed October 1, 2026, 1:18 AM ET / 05:18 UTC.

ClawSweeper review

What this changes

The PR includes saved sidebar channel orders and explicit clears in SQLite and PostgreSQL JSON exports, adds shared regression coverage, and updates documentation and the changelog.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked before merge - 4 items remain

This remains necessary on main, but the patch introduces an export failure for unmigrated v0.6.0 databases. No merged replacement resolves this work.

Priority: P2
Reviewed head: 5a1b75d1cb5c54b10f42d7fe4a085fe123b57a02

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) PR readiness rating was derived from proof quality, review findings, security review, and reviewer confidence.
Proof confidence 🌊 off-meta tidepool Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🧂 unranked krab (1/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Evidence reviewed 10 items Pinned introduced change: The pinned delta adds the sidebar table to both exporters, shared assertions, documentation, and a changelog entry; no schema or generated SQL changes are introduced.
Current main still omits preferences: Both main-branch export table lists omit user_sidebar_channel_order. The PostgreSQL implementation was inspected separately and has the same omission.
Export does not migrate: exportData opens the store and invokes ExportJSON without Migrate; neither backend’s Open performs migrations. The new unconditional SELECT therefore fails when the sidebar table has not been created.
Findings 1 actionable finding [P1] Preserve exports before the sidebar migration runs
Security None None.

How this fits together

ClickClack stores personal sidebar order in its account-preference tables. The operator’s export command reads database tables into a JSON snapshot for audits and migrations.

flowchart TD
 A[Saved account preferences] --> B[SQLite or PostgreSQL]
 C[Operator export command] --> D[Export table selection]
 B --> D
 D --> E[Read snapshot rows]
 E --> F[JSON output]
Loading

Before merge

  • Preserve exports before the sidebar migration runs (P1) - A database created by v0.6.0 lacks user_sidebar_channel_order, while exportData and both store Open methods do not run migrations. Adding this unconditional query therefore makes clickclack export fail with a missing-table error when an operator upgrades the binary and exports before starting the migrated server. The PostgreSQL addition has the same failure. Check table availability before querying this optional table and cover pre-migration databases on both backends.
  • Resolve merge risk (P1) - The new required table prevents exporting an unmigrated v0.6.0 database on either backend; compatibility must be restored and verified before merge.
  • Complete next step (P2) - Repair pre-migration export compatibility on both backends and provide redacted real export evidence before merge.
  • Improve patch quality - Address the highest-priority review finding and re-run the changed-surface validation.

Findings

  • [P1] Preserve exports before the sidebar migration runs — apps/api/internal/store/sqlite/export.go:25
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +2/-0; tests +35/-1; documentation +4/-0 Production growth is limited to the two export-table entries needed for the reported omission.

Merge-risk options

Maintainer options:

  1. Preserve pre-migration exports (recommended)
    Check for the optional sidebar table before querying it and add SQLite and PostgreSQL coverage using the v0.6.0 schema.

Technical review

Best possible solution:

Include saved preferences when their table exists while preserving exports of older databases without forcing schema changes.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes both the current-main omission and the introduced upgrade failure: export a v0.6.0 database with the candidate before running migrations. No runtime reproduction was executed during this read-only review.

Is this the best way to solve the issue?

No, the table-list repair is appropriately narrow but must tolerate an older schema; checking table availability preserves the existing export workflow.

Full review comments:

  • [P1] Preserve exports before the sidebar migration runs — apps/api/internal/store/sqlite/export.go:25
    A database created by v0.6.0 lacks user_sidebar_channel_order, while exportData and both store Open methods do not run migrations. Adding this unconditional query therefore makes clickclack export fail with a missing-table error when an operator upgrades the binary and exports before starting the migrated server. The PostgreSQL addition has the same failure. Check table availability before querying this optional table and cover pre-migration databases on both backends.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning medium; reviewed against c664dd71cfbb.

Labels

Label changes:

  • add P2: This is a bounded export-completeness repair with an actionable older-database compatibility defect.
  • add merge-risk: 🚨 compatibility: Unconditionally reading a post-v0.6.0 table breaks exports before database migration.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Label justifications:

  • P2: This is a bounded export-completeness repair with an actionable older-database compatibility defect.
  • merge-risk: 🚨 compatibility: Unconditionally reading a post-v0.6.0 table breaks exports before database migration.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Evidence

What I checked:

  • Pinned introduced change: The pinned delta adds the sidebar table to both exporters, shared assertions, documentation, and a changelog entry; no schema or generated SQL changes are introduced. (apps/api/internal/store/sqlite/export.go:25, 5a1b75d1cb5c)
  • Current main still omits preferences: Both main-branch export table lists omit user_sidebar_channel_order. The PostgreSQL implementation was inspected separately and has the same omission. (apps/api/internal/store/sqlite/export.go:23, c664dd71cfbb)
  • Export does not migrate: exportData opens the store and invokes ExportJSON without Migrate; neither backend’s Open performs migrations. The new unconditional SELECT therefore fails when the sidebar table has not been created. (apps/api/cmd/clickclack/main.go:596, 5a1b75d1cb5c)
  • Released database compatibility: v0.6.0 contains neither sidebar-order migration. Its two exporter files are identical to current main, so the PR adds a new table requirement to an export path that previously worked with that release’s schema. (31d299eee271)
  • Released migration absence: The tag has no entries for either sidebar-order migration. (31d299eee271)
  • Focused supplemental coverage: The shared contract compares exported rows with one saved order and one explicit empty order. Both callers initialize migrated stores, so this coverage does not exercise a pre-migration export. (apps/api/internal/store/storetest/sidebar.go:90, 5a1b75d1cb5c)

Likely related people:

  • Isaiah Knight: Raw commit 975ee58 adds apps/api/internal/store/storetest/sidebar.go:17 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 975ee58c1fc7; files: apps/api/internal/store/storetest/sidebar.go)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit 2408cf1 into main Oct 1, 2026
16 checks passed
@steipete
steipete deleted the round7/export-sidebar branch October 1, 2026 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant