-
Notifications
You must be signed in to change notification settings - Fork 31
ADR for backend data initialization #3734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughIntroduces a new ADR documenting a plan to shift initial data loading to the backend, generate bootstrap HTML in HomeController, switch to path-based routing (with legacy hash support), embed AppData and optional InitialInstanceData in the bootstrap payload, and remove frontend-led initial queries and certain components in a later task. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (5)
adr/002-backend-data-initialization.md (5)
112-121
: Use proper headings instead of bold for section titles (fix MD036).Convert the emphasized lines to markdown headings.
-**Positive** +### Positive @@ -**Negative / Trade-offs** +### Negative / Trade-offs
130-133
: Fix ordered list formatting in rollout plan.Numbers are missing periods/spaces and bold is misapplied.
-1**Route migration:** enable path-based routing; ship redirects for legacy hash URLs; validate deep links. -2**Delete old providers/effects** and RuleHandler/RuleConfiguration. -3**Docs & templates:** update app templates and docs; provide migration guide for app owners. +1. Route migration: enable path-based routing; ship redirects for legacy hash URLs; validate deep links. +2. Delete old providers/effects and RuleHandler/RuleConfiguration. +3. Docs & templates: update app templates and docs; provide migration guide for app owners.
94-100
: Avoid optional chaining in inline bootstrap and add null checks.Safer and more compatible DOM access; keeps behavior identical.
- <script> - window.AltinnAppData = JSON.parse(document.getElementById('__appdata').textContent); - </script> - <script> - window.AltinnInitialInstanceData = JSON.parse( - document.getElementById('__initialInstanceData')?.textContent || 'null', - ); - </script> + <script> + (function () { + var elApp = document.getElementById('__appdata'); + if (elApp && elApp.textContent) { + window.AltinnAppData = JSON.parse(elApp.textContent); + } + })(); + </script> + <script> + (function () { + var elInst = document.getElementById('__initialInstanceData'); + window.AltinnInitialInstanceData = elInst && elInst.textContent + ? JSON.parse(elInst.textContent) + : null; + })(); + </script>
78-86
: Clarify bootstrap schema versioning.Add an explicit version to the embedded contract to enable coordinated rollouts.
- **Bootstrap contract** + - Include an explicit `schemaVersion` in both `AppData` and `InitialInstanceData` to support backward/forward compatibility during deploys. @@ <script id="__appdata" type="application/json" > - { ... AppData ... } + { "schemaVersion": "1", ... AppData ... } </script>
103-106
: Routing ops: add server fallback and proxy-aware handling.Ensure SPA deep links and asset paths work behind reverse proxies/CDNs.
- Serve a single controller/action for all app routes with a catch-all and do not 404 on refresh.
- Honor app basePath when generating asset URLs.
- Return 404/403 statuses while still serving the app shell for client-side routing to show error pages, as appropriate.
- Verify redirects from legacy hash routes are relative and cacheable with short TTL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
adr/002-backend-data-initialization.md
(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
adr/002-backend-data-initialization.md
112-112: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
118-118: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
## Security & Compliance | ||
|
||
- Ensure embedded data contains only what the current principal may see. | ||
|
||
## Rollout Plan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Harden security: escaping, CSP, and caching directives for embedded JSON.
Embedding instance/app data in HTML has XSS and caching risks. Document the requirements here.
## Security & Compliance
@@
-- Ensure embedded data contains only what the current principal may see.
+- Ensure embedded data contains only what the current principal may see.
+- Escape sequences that could break out of the JSON/script context (e.g., `</script`), or use a safe JSON serializer that guarantees this.
+- Set a strict CSP with nonce/hashes; forbid `unsafe-inline`. Example: `script-src 'self' 'nonce-…'`.
+- Serve bootstrap responses with `Cache-Control: private, no-store` (at minimum for instance-specific payloads) and `Vary: Cookie, Authorization` to avoid proxy/CDN leaks.
+- Set `X-Content-Type-Options: nosniff`.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
## Security & Compliance | |
- Ensure embedded data contains only what the current principal may see. | |
## Rollout Plan | |
## Security & Compliance | |
- Ensure embedded data contains only what the current principal may see. | |
- Escape sequences that could break out of the JSON/script context (e.g., `</script`), or use a safe JSON serializer that guarantees this. | |
- Set a strict CSP with nonce/hashes; forbid `unsafe-inline`. Example: `script-src 'self' 'nonce-…'`. | |
- Serve bootstrap responses with `Cache-Control: private, no-store` (at minimum for instance-specific payloads) and `Vary: Cookie, Authorization` to avoid proxy/CDN leaks. | |
- Set `X-Content-Type-Options: nosniff`. | |
## Rollout Plan |
🤖 Prompt for AI Agents
adr/002-backend-data-initialization.md around lines 124-128: add concrete
security and caching requirements for any embedded JSON—require server-side
filtering to include only fields allowed for the current principal, serialize
with JSON.stringify and HTML-escape characters that can break HTML (<, >, &, /)
or prefix the payload (e.g., )]}',\n) to prevent JSON hijacking; mandate CSP
rules (script-src with nonces/hashes and default-src restrictions) and
X-Content-Type-Options: nosniff; specify caching policy: user-specific pages
must use Cache-Control: private or no-store and set appropriate Vary headers,
whereas truly public data may be cacheable with long max-age and immutable; add
automated tests and documentation notes for developers on safe embedding,
sanitization steps, and example headers.
- Update RuleHandler/RuleConfiguration to mention that this functionality will be removed in future versions. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve inconsistency: removal now vs. future removal.
Decision says remove RuleHandler/RuleConfiguration “in this major”, but acceptance criteria says “mention will be removed in future versions”. Align the plan.
-- Update RuleHandler/RuleConfiguration to mention that this functionality will be removed in future versions.
+- Remove RuleHandler/RuleConfiguration in this major and provide migration notes and deprecation notice in release docs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
- Update RuleHandler/RuleConfiguration to mention that this functionality will be removed in future versions. | |
- Remove RuleHandler/RuleConfiguration in this major and provide migration notes and deprecation notice in release docs. |
🤖 Prompt for AI Agents
In adr/002-backend-data-initialization.md around lines 153-154, the ADR
currently contradicts itself by stating RuleHandler/RuleConfiguration will be
removed "in this major" while the acceptance criteria says to "mention will be
removed in future versions"; update the document to make both statements
consistent: choose the intended timeline (remove in this major OR deprecate and
remove in a future version), then edit the decision and acceptance criteria so
they match that choice and reflect the exact migration/deprecation steps and
user-facing notices required.
When backend detects an instance context, it embeds (or exposes via single endpoint) the static-at-boot instance data adjacent to `AppData`, so the client does not immediately refetch/invalidate. Further interactions use targeted APIs. ([GitHub][1]) | ||
5. **Remove RuleHandler/RuleConfiguration in this major.** | ||
Do not migrate them to the new boot flow (handled by separate task). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also make a point of measuring the baseline, so that we can also measure the impact of these changes? 100% agreed on the principles of this, but would be even better if we had some data to point to. If we feel like auomated testing is out of scope, we could just include some manual measurements as text here based on a couple of waterfalls that are problematic/that we expect to improve. Examples
- Opening instance deeplink
- Render PDF
- ^ with and without subforms?
Even if it were just screenshots embedded into the markdown here it would help to motivate decisions and clarify which parts of the waterfalls are most important (or even not covered by the current proposal).
In the future we should have tests as code as well that can be ran to verify changes in performance over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you mention TTFB/TTI
, could include those perhaps
``` | ||
|
||
- **Routing:** | ||
- Backend maps `/` `/instances/:id/*` `/tasks/:taskId` etc. to a single `HomeController` action that embeds the correct initial payload and serves static assets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually possible to make this work with our current API routes? E.g. we have {org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid}
as a route in InstancesController
. Do we get a collision if we remove the hash? Or was the frontend route instance
(singular)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! The frontend route is singular; instance
.
Description
The Frontend Next task force is proposing to load initial app data on the backend.
Related Issue(s)
Summary by CodeRabbit