feat(db): Phase 2c-i — style_templates + style_rules + default style_rules seed#87
Merged
Conversation
Adds the schema and default seed for per-firm DOCX style templates so a future PR (#32) can override font, size, indent, spacing, bold, and caps per NodeType without touching generator code. Migrations (node-pg-migrate, reversible): - 010_style_templates: style_templates (UNIQUE name) + style_rules (UNIQUE template_id+node_type, FK CASCADE). OOXML-native units (half-points for font_size, twips for indent/spacing). - 011_seed_default_style_rules: one row in style_templates ('UFGS-Default') and 7 rows in style_rules (part, article, pr1..pr5) extracted from docs/references/UFGS/DIVISION_27/27080001.docx. Source values + extraction methodology recorded as a comment table in the migration file. Queries (src/db/queries/templates.ts): - getTemplate(id), getTemplateByName(name), listTemplates(), createTemplate(name, owner?), upsertStyleRule(templateId, rule) - All re-exported from src/db/index.ts (barrel). Csi → Spec rename (in-scope cleanup): - buildCsiNumberingConfig → buildSpecNumberingConfig - CSI_NUM_REF → SPEC_NUM_REF - 'csi-numbering' literal → 'spec-numbering' - Generator output is functionally identical; reference string renamed for internal consistency. Verified: zero grep hits for old names. Other: - .gitignore: docs/references/UFGS/**/*.docx (fixture stays local; values that matter are encoded in 011's comment + INSERT statements). - ARCHITECTURE.md: added style_templates + style_rules to schema overview. Closes #30 Test plan: - pnpm migrate / pnpm migrate:down round-trip both migrations cleanly - pnpm test (462 unit tests pass) - pnpm test:integration (117 pass, 65 pre-existing skips) — includes new templates.integration.test.ts covering seed, CRUD, upsert idempotence, FK cascade, UNIQUE name constraint - pnpm lint clean
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Phase 2c-i DB schema and seed for style templates/rules, a typed query API with re-exports and integration tests, updates ARCHITECTURE and .gitignore, and renames the DOCX numbering config to use ChangesStyle Template Database & Query API
Generator Numbering Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (1)
src/db/queries/templates.integration.test.ts (1)
12-25: ⚡ Quick winAvoid mutable shared state for created template tracking.
This uses in-place mutation (
push,length = 0,splice) on module-level state. Prefer immutable reassignment (let+ spread/filter) to keep tests deterministic and guideline-compliant.As per coding guidelines: "Use immutable patterns: create new objects, never mutate. Use spread operators, not property assignment."
Also applies to: 159-160
🤖 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/db/queries/templates.integration.test.ts` around lines 12 - 25, Replace the mutable module-level array usage for tracking created templates (CREATED_TEMPLATE_NAMES, trackName, afterEach cleanup) with an immutable pattern: change CREATED_TEMPLATE_NAMES to a let-bound collection and update trackName to return a new collection reference instead of pushing (e.g., reassign CREATED_TEMPLATE_NAMES = [...CREATED_TEMPLATE_NAMES, name] inside trackName or return the new array and assign the result), and in afterEach perform cleanup by reassigning CREATED_TEMPLATE_NAMES to a filtered/new empty array (e.g., CREATED_TEMPLATE_NAMES = CREATED_TEMPLATE_NAMES.filter(...) or = []) rather than using in-place mutation like push or length = 0; ensure tests use the reassigned variable consistently so no in-place mutations remain.
🤖 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/db/migrations/010_style_templates.ts`:
- Around line 19-21: Update the migration 010_style_templates.ts to enforce
DB-level constraints: add a CHECK on the node_type column (node_type)
restricting values to the allowed set
('part','article','pr1','pr2','pr3','pr4','pr5') and add CHECK constraints that
the OOXML numeric fields (the numeric style columns defined in this
migration—e.g., font_size / run_font_size, indent, spacing_before, spacing_after
or whichever numeric columns are declared in lines 23-30 and 33-35) are >= 0;
modify the CREATE TABLE / addColumns logic in this migration so the column
definitions include these CHECK constraints (and update the corresponding down
migration to drop these constraints if present).
In `@src/db/queries/templates.ts`:
- Around line 3-5: Tighten the type for node types by introducing a finite union
type (e.g. StyleNodeType) and replacing all occurrences of plain string with
that union: change the StyleRule.nodeType property from string to StyleNodeType,
update any other declarations that use nodeType (including the referenced usages
around the other occurrences and the upsertStyleRule function signature) to
accept StyleNodeType, and export/reuse the new StyleNodeType type so callers
must supply only the allowed node kinds.
---
Nitpick comments:
In `@src/db/queries/templates.integration.test.ts`:
- Around line 12-25: Replace the mutable module-level array usage for tracking
created templates (CREATED_TEMPLATE_NAMES, trackName, afterEach cleanup) with an
immutable pattern: change CREATED_TEMPLATE_NAMES to a let-bound collection and
update trackName to return a new collection reference instead of pushing (e.g.,
reassign CREATED_TEMPLATE_NAMES = [...CREATED_TEMPLATE_NAMES, name] inside
trackName or return the new array and assign the result), and in afterEach
perform cleanup by reassigning CREATED_TEMPLATE_NAMES to a filtered/new empty
array (e.g., CREATED_TEMPLATE_NAMES = CREATED_TEMPLATE_NAMES.filter(...) or =
[]) rather than using in-place mutation like push or length = 0; ensure tests
use the reassigned variable consistently so no in-place mutations remain.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23a84883-3a10-415e-94d0-d9230f78015f
📒 Files selected for processing (10)
.gitignoreARCHITECTURE.mdsrc/db/index.tssrc/db/migrations/010_style_templates.tssrc/db/migrations/011_seed_default_style_rules.tssrc/db/queries/templates.integration.test.tssrc/db/queries/templates.tssrc/generator/index.tssrc/generator/numbering.test.tssrc/generator/numbering.ts
…e_rules
Tightens both the TypeScript and SQL boundaries for style_rules.node_type
so invalid kinds can't enter the table from either the query layer or a
direct INSERT.
- Introduce StyleNodeType union ('part' | 'article' | 'pr1'..'pr5') in
src/db/queries/templates.ts; export it (and STYLE_NODE_TYPES) from
src/db/index.ts. StyleRule.nodeType and StyleRuleRow.node_type now use
this union — upsertStyleRule callers get compile-time rejection of
invalid kinds.
- Migration 011 mirrors the union as a local SeedNodeType so the seed
data is statically verified against the same domain.
- Migration 010 adds two CHECK constraints on style_rules:
* node_type ∈ ('part','article','pr1'..'pr5')
* font_size_half_pt / indent_twips / space_before_twips /
space_after_twips are NULL or >= 0 (OOXML units are unsigned)
The down migration drops the table, so the constraints come along.
- Integration test ruleFor() helper now takes StyleNodeType instead of
string so test fixtures stay aligned with the production type.
Addresses CodeRabbit review on #87.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/db/migrations/010_style_templates.ts`:
- Line 6: The migration defines the style_templates table with name: { type:
'text', notNull: true, unique: true } but does not prevent empty strings; update
the table creation in 010_style_templates.ts (the CREATE TABLE / up migration
where style_templates is defined) to add a check constraint preventing empty
names — e.g. add a constraint that name <> '' or char_length(name) > 0 (or
equivalent check expression supported by your SQL/ORM) so the name column
remains not-null, unique, and non-empty.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fd8582c-8b1d-4aa4-bb3b-cf9b681666f5
📒 Files selected for processing (5)
src/db/index.tssrc/db/migrations/010_style_templates.tssrc/db/migrations/011_seed_default_style_rules.tssrc/db/queries/templates.integration.test.tssrc/db/queries/templates.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/db/index.ts
- src/db/migrations/011_seed_default_style_rules.ts
- src/db/queries/templates.integration.test.ts
- src/db/queries/templates.ts
Adds a CHECK constraint `style_templates_name_non_empty_check` on `style_templates.name` enforcing `length(trim(name)) > 0`. Without it, the column accepted '' and ' ', either of which would silently break `getTemplateByName()` lookups and leave orphaned style_rules behind. Verified locally: INSERTs with '' and ' ' now fail with the named constraint; a 'valid' name still inserts. Down migration drops the table, so the constraint comes along. Addresses CodeRabbit minor on #87 (latest re-review).
# Conflicts: # src/db/index.ts
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.
Summary
style_templates+style_rulesschema (migration 010) and a UFGS-derived default seed (migration 011) so a future PR (feat(generator): Phase 2c-iii — wire templateId through generator #32) can override per-NodeType font/size/indent/spacing/bold/caps in DOCX output without code changes.src/db/queries/templates.tswithgetTemplate,getTemplateByName,listTemplates,createTemplate,upsertStyleRuleand re-exports them from thedbbarrel.buildCsiNumberingConfig→buildSpecNumberingConfig,CSI_NUM_REF→SPEC_NUM_REF,'csi-numbering'→'spec-numbering'). Output is functionally identical; verified zero remaining grep hits.docs/references/UFGS/**/*.docxto.gitignore— the fixture (27080001.docx) stays local; its extracted values are encoded in migration 011's comment table + INSERT statements so a reviewer can verify the mapping without re-running extraction.ARCHITECTURE.mdschema overview with the two new tables.Design doc (authoritative — supersedes the original issue body which said
.sqlmigrations):docs/superpowers/specs/2026-05-18-issue-30-design.md.Schema decisions vs original issue body
font_size_ptfont_size_half_pt INTEGERspace_*_pt*_twips INTEGERcapsBOOLEAN NOT NULL DEFAULT falsew:caps(template_id, node_type)UNIQUEstyle_templates.nameUNIQUE'UFGS-Default'boldNOT NULL DEFAULT falseOut of scope (per design doc)
ownerisolation — feat(api): Phase 5f — authentication + multi-tenant (JWT, org isolation) #43Test plan
pnpm migratesucceeds;pnpm migrate:downreverses 011 then 010 cleanly; re-apply succeedspnpm test— 462 unit tests pass (numbering.test.ts updated to use renamedbuildSpecNumberingConfig)pnpm test:integration— 117 pass, 65 pre-existing skips. Newtemplates.integration.test.tscovers: UFGS-Default has exactly 7 rules; each NodeType present once;partrow matches extracted values (Courier New, sz=20, bold=true, caps=true,'PART %1 -');getTemplatereturns null for unknown id;createTemplateround-trips;upsertStyleRuleis idempotent (insert then update); FK CASCADE deletes child rules; UNIQUE name constraint rejects duplicatespnpm lintclean (eslint + tsc + prettier)grep -rn 'csi-numbering\|buildCsiNumberingConfig\|CSI_NUM_REF' src/returns zero hits.docxfixture is gitignored (not in the commit)Closes #30
Summary by CodeRabbit
New Features
Documentation
Tests
Chores